I'd want to have a wider view on the class model if I were actually doing a full code review, but these all look like very obvious cases where one should definitely use a property rather than a method.
ActiveFilters first. If I see a read-only property on an object called
ActiveFilters, then I expect that "Gets the active filters for the object" to be what I'd see if I looked in the documentation; principle of least surprise therefore says that if that's what this property does, then all is good, otherwise it may not be.
And it is.
It happens to be a memoised property; that is to say, it calculates the value to return on first call and caches the result for subsequent calls. This is an optimisation for the case where a property is likely to be called more than once (otherwise it saves nothing). It's most useful in cases where the property might sometimes not be accessed (because in such cases the code never runs) though even when the property's getter is called every time, it's still nice to keep the logic for obtaining the value near to the place where it is accessed; aiding ones ability to understand the code.
Of course, outside calling code doesn't know whether it is memoised, a read from a field with no further logic, or if it calculates the result on each call.
Nor does calling code know if that memoisation is permanent, or if there is a method or setter elsewhere in the class that might either re-fill
_activeFilters, or set it to null to force recalculation.
This is one of the main reasons for using properties rather than just exposing fields—if they'd exposed a field (and ensured it was correctly filled on construction) it would be a breaking change to later change to something like this.
It looks reasonably robust (I'd want to examine the details of
SrpDto.Widgets etc. and know about why there is one item being skipped before I'd say anything more firmly than "reasonably").
All in all a textbook case of a memoised property getter.
If I were to critique it at all, it would be that perhaps rather than return a
Lists that could then be changed, it might be better to return a readonly
ReadOnlyCollection or copies of the lists. Another alternative design would be to make it an indexed getter that returns read-only versions of the list based on a string key. However:
- This might be a reasonable saving for the developer writing it, developers using it, and the processing computer, especially if it's an internal class.
- It might be absolutely the perfect thing to do; I can't know it's not without seeing more.
- The implementation would still be much like the above anyway, but with some extra work added to handle the creation of a readonly dictionary or handling of indexing.
(I'd also want to see the name
Widgets justified; if it relates to a functional UI element, then it's perhaps okay, but I sometimes see it used with no further justification than "oh, I just wanted to use a silly piece of 1930s slang to make this less readable").
SearchBarLayout is another textbook memoised property getter. I'm not so sure about the naming convention that gives us Constants.SearchBarLayout (that had really better be an internal class or enum) but as far as being a property getter goes; of course it should, what else would it be?
SearchBarTemplate is even more obviously something that should be a property; it maps a set of values of one type to a set of values of another. Perhaps that logic should be elsewhere, but perhaps not (again, I can't tell from just here; the main thing is whether that logic is being duplicated all over the place). Still, that's not relevant to the question here; in terms of getter vs. method, this one's an obvious getter.
There's a bit of a bad smell in the use of a switch performing an arbitrary-looking mapping, as that is often something that can be done better another way. However;
- Sometimes it is just the best way to do it.
- If it can be improved, it can be improved as a non-breaking change, in this spot.
In all, while there is a degree of judgement in the question of whether something should be a property or a method, there are some general principles:
- If we think of classes and struct as nouns, objects as instances of those nouns, and methods as verbs (and static classes as abstract nouns), then properties should be things we can reasonably think of as adjectives or prepositions (describing a relationship between the object we called on and the object we got or set).
- Getters should generally not throw due to the caller doing an incorrect choice (throwing due to e.g. a database connection going down is another matter). If they do they should do so in a way that is obvious from knowledge of the class/struct and can test for (e.g. the way
InvalidOperationException) or because the property is indexed and the error relates to the key.
- Getter should not change state as seen from the outside, only report on what the state is. (Memoisation changes internal state, but as seen from the outside it's no different than calculating the result each time, or having already calculated it).
- A setter must throw on invalid values being set. This is their primary reason for existing (rather than just exposing fields), though another strong reason is that they can update more than one piece of state at a time.
- Ideally getters and setters should be pretty fast (like all those shown above) especially if amortised over several calls (again, like the first two of those above). There's no semantic reason for this, but having what looks like it should be a property actually be a method can be a subtle warning that something is expensive (e.g.
DateTime.UtcNow is calls whatever methods the underlying OS has for obtaining the current time such as 'GetSystemTime
followed bySystemTimeToFileTime` on Windows, so there's no reason that wouldn't be an obvious thing to make a property, but a method for contacting several atomic clocks around the world and then deducing network delays to give a good estimate of the current time, might be better off being a method).
Another way of looking at the question, is to consider properties to be "smart fields", again like all the examples you give.
I don't see how anyone would object to these getters. I would be suspicious that anyone who did might be tending to use methods when they should really use properties.