During a recent merry development team get together, we set out with an agenda of five things to talk about, which then soon descended into a vigorous debate about something not on the agenda.
We were reading through some code, when one of the more seasoned developers on our team raised a concern about the following snippet of code. (The development stack is TypeScript/Angular4), but that's not really relevant to the discussion):
get someIsValidProperty(): Number {
return this.ruleService.runSomeRule();
}
In this instance, a field on a template form references someIsValidProperty. In the template is looks like a simple call out to some property that indicates a valid or invalid state. In this respect the argument was that it looks neat and intuitive, and was ultimately calling through to some logic, that we'd decided to move out and make a dependency.
However from a unit test perspective, it looks plain weird. As we're asserting that the mock of the ruleService is being called with the expected values. The feeling was that the getter should really have been a method instead, and that would have been much clearer in the client calling context. Moreover, a further argument was that getters and setters should really expose in some way the internal state of the class/object itself, rather than hand off to something else.
It's fair to say that we were polite, but divided, and resolved to 'go and see what the other teams are doing and copy them ®'
Now I expected to fire up google when I got back to my desk, and find a whole raft of cunning arguments either way to further enrage empower the respective camps.
But there was nothing (that I could find within the confines of my own short attention span).
So, my question is this really, are there any reasons, philosophically or otherwise, why we should avoid calling methods from within getters or setters? In the spirit of avoiding opinion based discussion, some clear thoughts and arguments would be appreciated.
I think this is all very much an aesthetic and practical decision in the end, more than a general universal guideline. Properties are syntactic sugar to save like five characters, and you can make what you want of that. I'd personally say it's fair to make into a property whatever that "appears to belong" to the object from the client's perspective. The implementation can be then take any number of shapes (from an actual field in the object, a getter of a private field or a call to a method of another object). This is very practical for making testing fakes. However, when I say "it's fair" I don't necessarily mean that's what I'd always do. A big project developed by a team should prioritise consistency and productivity. I think it is also fair to say that properties can be awkward to test, and so shouldn't contain any logic requiring testing.
One can consider it from both points of view to decide for a compromise. From the point of view of the template designer (coder or not), properties make things "neater", which is not as trivial as it may sound, because neater templates are easier to maintain. From the point of view of the tester, properties may make "worse-looking" tests (again, reduced maintainability). You have a continuum from "no properties" to "anything can go in properties", and the team can decide where to put a line where everyone is comfortable, but the important thing is that this is made explicit as a guideline (e.g. "do not put logic into properties" or "remember to test properties because they may contain logic").
A possible "best of both worlds" solution is to have a rule like "properties can only get/set a private field or call a getter/setter method". So you have:
getSomeIsValidProperty(): Number {
return this.ruleService.runSomeRule();
}
get someIsValidProperty(): Number {
return this.getSomeIsValidProperty();
}
So you have neat templates and good-looking tests. Of course, the drawback is that you have somewhat redundant code (even though properties would never need to be tested).