Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

This question already has an answer here:

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;
            }
        }
    }
share|improve this question

migrated from stackoverflow.com Jan 24 at 16:23

This question came from our site for professional and enthusiast programmers.

marked as duplicate by Shog9 Jan 24 at 16:23

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

1  
This probably belongs on either Programmers or CodeReview, but in any case, you're probably right in this case, because it's a violation of the principle of least surprise. Properties which can throw exceptions are generally a code smell. Validation or notification are fine, of course. –  Magus Jan 23 at 17:23
    
It's horribly unclear for one thing. myClassObj.MyProperty = 1 that triggers some kind of action to occur (without a PropertyChanged of course) is unexpected at best. Properties = values in my opinion, and leave behavior to methods. –  DonBoitnott Jan 23 at 17:23
    
Thanks Magnus I added the code review tag. –  JBeckton Jan 23 at 17:43
    
@DonBoitnott That will never happen here; these are all read-only properties. –  nmclean Jan 23 at 18:47
    
Off Topic? I asked for a code review of some C# code using the code-review tag. So what is a code review? This is why I try very had not to post questions on this site anymore. As best as I can understand the help section on this this is indeed on topic. Who Moderates the Moderators? –  JBeckton Jan 24 at 1:54

3 Answers 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.

share|improve this answer

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.

share|improve this answer
    
Maurice.. thanks for your input but your reference is for .net 1.1, I think better practices have evolved since then. here is a newer version msdn.microsoft.com/en-us/library/ms229006.aspx "Property getters should be simple operations and should not have any preconditions. If a getter can throw an exception, it should probably be redesigned to be a method. Notice that this rule does not apply to indexers, where we do expect exceptions as a result of validating the arguments." –  JBeckton Jan 23 at 18:13
    
True enough, but it's still in their standards doc. Even in later documents Microsoft says that "methods represent actions and properties represent data". That linked document shows a property getter that calls methods. I still say it's not a bad practice per se, and more a matter of preference. –  Maurice Reeves Jan 23 at 20:26

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.

share|improve this answer