Hiding inherited members

Today I’d like to talk about a bug I discovered in our code base at work and some of the lessons I learnt along the way. It’s one I’ve never come across before but it’s something I’ll surely remember the next time I need to think about a new class hierarchy. The bug revolves around hiding inherited members in C# and how values can seemingly disappear when being passed around between methods.

This bug took me about half an hour to find, three hours to attempt refactoring it, one hour to roll back those changes and just fix the bug reported by the business and a further 30 minutes to deploy and test the change in staging and push out a fix to production. Hopefully I can save you 5 or so hours by trying to avoid this pattern or at least convince you to write unit tests to give you some level of confidence.

Here are the class definitions.

class LoggingClass {
    public object OldValue { get; set; }
    public object NewValue { get; set; }
}

class IntLoggingClass : LoggingClass {
    public new int OldValue { get; set; }
    public new int NewValue { get; set; }
}

class DateLoggingClass : LoggingClass { } // You get the idea

And here is how it was being used.

class ChangeLogger {

    public LoggingClass ConstructLogObject(object oldValue, object newValue, ItemLogTypeEnum itemtype) {

        LoggingClass item;

        switch (itemtype)
        {
            case ItemLogTypeEnum.Int:
                item = new IntLoggingClass() {
                  OldValue = oldValue,
                  NewValue = newValue
                };
                break;

            case ItemLogTypeEnum.Date:
                // You get the idea
        }

        return item;
    }
}

So the code appears harmless and its not immediately obvious when you’re reading it first thing in the morning, but the due to the way the properties of the IntLoggingClass have been declared using the new keyword, the values stored in OldValue and NewValue are never received by callers. Declaring members using the new keyword hides (or shadows) the base class members them which brakes inheritance, using its own custom implementation.

To illustrate one of the problems;

LoggingClass log = new IntLoggingClass() {
    OldValue = 0,
    NewValue = 1
};

Console.WriteLine (log.NewValue); // null
Console.WriteLine (log.OldValue); // null

And in some cases;

LoggingClass log = null;

if (somecondition) {
    log = new IntLoggingClass();
}

// Values are assigned correctly
log.OldValue = 1;
log.NewValue = 2;

// Back up the call stack...

var logcast = log as IntLoggingClass; // Values are lost during a reference conversion

if (logcast != null) {
    Console.WriteLine (logcast.NewValue); // 0
    Console.WriteLine (logcast.OldValue); // 0
}

Now I could talk about the use of generics to solve this whole mess or how unit tests could have caught this earlier but that’s not the topic of this post. And to be fair hiding members exists in C# for a reason and might be useful in some situations, but as you can see it can also be dangerous.

The mistake I made was diving straight in to refactoring, thinking I could not only fix the bug reported but also “fix” a whole bunch of other bugs which no one had noticed so far. My main priority should have been to fix the bug which was reported and log another issue (which I did end up doing) in our issue tracking software. I wanted so bad to fix it all up that I lost sight of how large the problem was.

There were expressions checking the type of class if (type == typeof(IntLoggingClass)) and then doing something custom, method calls and call stacks had to be changed, external dependencies had to be updated (WCF), and how could I possibly test an entire code base without unit tests giving me some level of confidence? It’s not possible without a concerted effort. When you’re changing that much code in one go your almost certainly going to be wading through lots of compiler errors, fixing them one by one until the compiler gives you the all clear, only to then realise that you have to deal with runtime exceptions and broken external dependencies.

Here’s a case where I had to settle for a “quick fix” and keep moving on to other work. Although this issue was logged in our issue tracker so that someday “we will get around to fixing it”, something tells me there will be more important things between now and whenever that is.