Fields and virtual members

It seems like the sillier the bug, the more time you’ll spend debugging it, simply because it’s in functionality you just knew that worked correctly (and had the tests to back it up).  One problem we hit recently was code that used fields to back virtual properties:

public abstract class Enumeration : IComparable
{
    private readonly int _value;

    protected Enumeration() { }

    protected Enumeration(int value)
    {
        _value = value;
    }

    public virtual int Value
    {
        get { return _value; }
    }

    public virtual int CompareTo(object other)
    {
        return _value.CompareTo(((Enumeration)other).Value);
    }
}

This isn’t a problem in and of itself, except in the behavior of that last method.  Notice its logic uses the field and not the property.  I can then define a derived type:

public class PercentDiscount : Enumeration
{
    public override int Value
    {
        get { return 5; }
    }
}

But now my CompareTo method breaks because it isn’t using the correct value.  It uses the uninitialized field, causing all sorts of bizarre behavior.  When you have logic errors in places like GetHashCode, Equals or CompareTo, I can guarantee your application will break in the most hair-pulling ways imaginable.

Let’s fix it

So what’s a better option?  We could just “make sure” that we don’t use the private field, and only use the virtual property.  But I hate conventions that rely on my memory to not make a mistake, I want to fall into the pit of success.  One way to do so is to use automatic properties:

public abstract class Enumeration : IComparable
{
    protected Enumeration() { }

    protected Enumeration(int value)
    {
        Value = value;
    }

    public virtual int Value
    { 
        get;
        private set;
    }

    public virtual int CompareTo(object other)
    {
        return Value.CompareTo(((Enumeration)other).Value);
    }
}

Instead of a backing field, we’ll use an automatic property, making it impossible to use a non-virtual backing field.  We still get the same access modifiers (a virtual, overridable getter, a private setter), plus the benefits of automatic properties.  So now I’m curious: are there any valid reasons to keep using fields for holding state?

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Jimmy Bogard

I'm a technical architect with Headspring in Austin, TX. I focus on DDD, distributed systems, and any other acronym-centric design/architecture/methodology. I created AutoMapper and am a co-author of the ASP.NET MVC in Action books.
This entry was posted in C#. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • Akash Chopra

    You still need fields if you want them to use the readonly modifier :(

  • http://shadowcoding.blogspot.com Erik

    You also need backing fields if you intend to do something with the property value inside the getter or setter. It would be *so* nice to be able to use the ‘value’ keyword in this way, both in the getter and setter…

  • http://realfiction.net Frank Quednau

    With wholly unexposed state I have no quarrel with fields. I also like using the readonly modifier for state that is immutable after instance creation

  • Rob

    This is why I’d like to see a form of property that has the backing field within the property description like so:

    public int MyProp
    {
    int _myProp;
    get { return _myProp; }
    set { _myProp = value; }
    }

    That way you could still do all the custom get/set code that you want without exposing the backing field.

    An alternative would be to add a new keyword like “value” to represent the auto created backing store value.

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Akash, Frank

    Yah, I forgot about the readonly modifier. Though, I typically only use that for services (dependencies), collection types, or immutable objects.

    @Rob et al

    I see properties with logic as a bad idea. Methods are more expressive that extra things are happening when getting or setting values.

  • Paco

    Now you have a bug in the constructor of an inheritor when you override the virtual method (and break the LSP)

  • AlexEzh

    Why not set _value to 5 which would keep the existing class logic? I understand that the class is an example, but you are trying to hide the behavior of the base class which is a violation of Liskov substitution principle. Article on Wikipedia has a good example.

  • Christopher Harris

    @Paco: I have to admit you make a very interesting observation on breaking the LSP. It makes me wonder if this is one of those cases where a new trivial unit test would have been more appropriate to catch the CompareTo bug. (Clean Code: Smell T3: Don’t Skip Trivial Tests)

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Paco et al

    LSP isn’t violated here. Fields are an implementation detail and should never be exposed outside of that class (i.e., instance fields should always be private).

    It’s just one of those cases where if a class exposes state, it should be self-consistent and use the property accessors.

  • AlexEzh

    I would say differently. It is fine to use fields in the class implementation and expose property accessors as long as accessors are not virtual. If virtual accessor is required, than ideally it should be abstract to avoid unnecessary fields in the class implementation.

  • http://autoregistration.codeplex.com/ Artem Govorov

    Presentation frameworks (WinForms, WPF) UI elements 2-way data binding heavily depends on read/write properties and INotifyPropertyChanged interface implementation, so property setter is an obvious place to put code raising property changed event. I’m not saying that this is good or bad – it just the way it is and this makes it impossible to use auto properties in these cases.

  • Fizz

    I want Property scoped values, solves all of these problems

    public int MyProperty
    {
    int myField;
    get { return myField; }
    set
    {
    if (!MyFieldValid(value) throw new Exception(“”);
    myField = value;
    NotifyPropertyChanged(“MyProperty”);
    }
    }

    MS does not think this is usefull

  • Ryan R

    How about lazy loading? I have yet to see a good way (no reflection) of lazy loading an automatic property.

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Ryan

    Do you mean manual lazy loading? I haven’t done that kind of code in a long, long time.

  • Mahesh

    Since property is converted into method in IL, there could be slight performance hit as it is method call.

  • Dan Puzey

    While I agree that you shouldn’t have “behaviour” in a property, it’s not difficult to think of cases where you need logic in a property: I’d expect most setters to have some basic validation in them – checking for null, or negative numbers or whatever. That typically requires a backing field, unless there’s some syntax I haven’t learnt yet (which is entirely possible :-) )