Logic inside class properties setters & getters

Question

Trying to build a case against this as we have a developer that likes to use class properties in place of methods. I think it's a bad idea. In some cases he is accessing the data layer inside the setter and getter.

here is a snippet of code, what are your thoughts on this?

private Dictionary<string, List<string>> _activeFilters;
    public Dictionary<string, List<string>> ActiveFilters
    {
        get
        {
            if (_activeFilters == null)
            {
                _activeFilters = new Dictionary<string, List<string>>();
                var qs = HttpContext.Current.Request.QueryString;
                foreach (var widget in SrpDto.Widgets)
                {
                    _activeFilters[widget.SearchParameter] = qs[widget.SearchParameter] != null ? qs[widget.SearchParameter].Split(',').Skip(1).ToList() : new List<string>();
                }
            }

            return _activeFilters;
        }
    }

    private Constants.SearchBarLayout? _searchBarLayout;
    public Constants.SearchBarLayout SearchBarLayout
    {
        get
        {
            if (_searchBarLayout == null)
            {
                var searchBarLayoutType = typeof(Constants.SearchBarLayout);

                // Use the first enum value as the default if no module setting is present
                _searchBarLayout = (Constants.SearchBarLayout)Enum.Parse(searchBarLayoutType,
                    this.ModuleContext.Settings[Constants.SRPSearchBarLayoutKey] as string ?? Enum.GetName(searchBarLayoutType, 0));
            }

            return _searchBarLayout.Value;
        }
    }

    public string SearchBarTemplate
    {
        get
        {
            switch (this.SearchBarLayout)
            {
                case Constants.SearchBarLayout.SimpleSearchBar:
                    return Constants.SRPSimpleSearchTemplateName;
                    break;
                case Constants.SearchBarLayout.FacetedSearchBar:
                    return Constants.SRPFacetSearchTemplateName;
                    break;
                default:
                    return string.Empty;
            }
        }
    }

Answer 1

I'm going to say that in some cases, this is exactly what you want. Having logic in the property get/set is not unheard of, and in fact, the Microsoft guidelines for properties show calling a method to make a control active when a value changes.

I can think of many scenarios where you'd want to retrieve the values from the database when calling get, and in the cases above, the properties are read-only because they do not have a set method defined, so you wouldn't inadvertently update something in your code and break the principle of least surprise.

On the flip side, if every action that calls to the database or has some logic in it to determine a value must be a method, what would you call the method? GetActiveFilters()? That sounds like it should be a property.

By all means you can set a different coding standard for your own team if this is not what you prefer, but given that Microsoft itself adopts and demonstrates ways to bundle logic into property getters and setters, I'd say go easy on the guy, and tell him that your preference is you don't do it this way.

Answer 2

If there was no logic inside getters and setters, then what would properties be? SearchBarLayout is a simple conditional, and I think it's fine. The others could be more debatable, but this is a common pattern:

private T _value;
public T Value {
    get {
        if (_value == null) {
            _value = [initialization logic for the field]
        }
        return _value;
     }
}

This means that if someone instantiates this class but never accesses Value, time was saved by skipping the initialization logic. And if it is accessed more than once then it's still only processed once (except in cases where the initialization could return null). It also keeps the code more sequentially organized than it would be if the fields / properties were listed and XML-documented in one section and initialized in other methods.

You have to decide whether those benefits actually apply. Maybe they are doing separate calls to the data layer that could be more efficient as one call so you should initialize both fields at once? I don't know the details, but personally I wouldn't have a problem writing code like you posted.

Answer 3

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.

Let's consider 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 Dictionary containing Lists that could then be changed, it might be better to return a readonly IDictionary or 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:

1. 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.
2. It might be absolutely the perfect thing to do; I can't know it's not without seeing more.
3. 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;

1. Sometimes it is just the best way to do it.
2. 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:

1. 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).
2. 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 default(int?).Value throws InvalidOperationException) or because the property is indexed and the error relates to the key.
3. 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).
4. 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.
5. 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 'GetSystemTimefollowed 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.