bad programming style to change the state of the object by using the get accessor RRS feed

  • Question

  • Hello

    I had a doubt as to why the following code is a bad programming style . It is from MSDN Documentation

    It is a bad programming style to change the state of the object by using the get accessor. For example, the following accessor produces the side effect of changing the state of the object every time that the number field is accessed.

    private int number;

    publicint Number { get { return number++; // Don't do this } }


    Manoj Gokhale

    Friday, August 31, 2018 6:01 AM

All replies

  • It's just because accessing a property should not change the state of an object.  I should be able to fetch a property repeatedly and get the same answer.  It also makes concurrent processing more difficult, because you can't predict which thread might get in first.

    There are exceptions to every rule, of course.  If you have an object that is delivering a unique ID, and you document the fact that the property access modifies the state, then of course you can do it, although it might be better to make it a method instead of a property.

    Tim Roberts | Driver MVP Emeritus | Providenza & Boekelheide, Inc.

    Friday, August 31, 2018 6:32 AM
  • I think Tim covered that fairly well, but I'd like to add a couple of cents' worth.

    If code has side-effects, that can make it difficult to maintain. For example, if your code appears to be just reading a property, when you are looking at the code you will expect that to be all it does. If it does something else at the same time - like incrementing a number - you might miss the effect and not know what's going on.

    Friday, August 31, 2018 7:08 AM
  • If you refer to the Design Guidelines from MS for libraries (which all code should follow) then it'll state 2 guidelines related to properties.

    1) Properties are deterministic

    2) Properties behave like fields

    Devs don't cache property values when they call them. Properties should be fast and deterministic. This should work correctly every time.

    var original = someObject.SomeProperty;
    for (var x = 0; x < 10; ++x)
    var areEqual = original == someObject.SomeProperty;

    If a property is not deterministic then a dev has to cache the value. Devs aren't used to that so they won't. Properties look like fields and will be treated as such. When a dev gets different results each time (without an intermittent write) they will assume there is a bug. If a value will change each time (aka it isn't deterministic) then it should be a method. Devs know that if they want to keep the results of a method they have to store it. 

    Note that the guidelines were written after .NET 1.0 was released. As an example of not how to do properties they always point back to DateTime.Now. This is a non-deterministic property. Every time you call it you get a new value. It should have been a method.

    Note also that the guideline doesn't say data cannot be changed. It says it should have no side effect and refers to public side effect. It is very common, and recommended, that properties that are delay initialized or expensive to evaluate be cached. So it is common for a getter to initialize/cache a properties value. But thereafter the same value gets returned until the state is refreshed some other way (e.g. FileInfo.Refresh).

    Also note that there may be boundary cases where it makes sense to violate the guideline. It should be clear though that this is happening. A good example is Stopwatch. It has an ElapsedTick property. Given it is a stopwatch and the property is Elapsed it makes sense that it is non-deterministic. 

    Michael Taylor http://www.michaeltaylorp3.net

    • Proposed as answer by BonnieBMVP Saturday, September 1, 2018 4:45 PM
    Friday, August 31, 2018 3:12 PM