C#: Using virtual leads to confusion?
A colleague and I were looking through some code that I worked on a couple of months ago where I had created a one level hierarchy using inheritance to represent the response status that we get back from a service call.
The code was along these lines:
public class ResponseStatus
{
public static readonly ResponseStatus TransactionSuccessful = new TransactionSuccessful();
public static readonly ResponseStatus UnrecoverableError = new UnrecoverableError();
public virtual bool RedirectToErrorPage
{
get { return true; }
}
}
public class UnrecoverableError : ResponseStatus
{
}
public class TransactionSuccessful : ResponseStatus
{
public override bool RedirectToErrorPage
{
get { return false; }
}
}
Looking at it now it does seem a bit over-engineered, but the main confusion with this code is that when you click through to the definition of 'RedirectToError' it goes to the ResponseStatus version of that property and it's not obvious that it is being overridden in a sub class, this being possible due to my use of the virtual key word.
You therefore need to look in two places to work out what's going on which isn't so good.
A solution which we came up with which is a bit cleaner is like so:
public abstract class ResponseStatus
{
public static readonly ResponseStatus TransactionSuccessful = new TransactionSuccessful();
public static readonly ResponseStatus UnrecoverableError = new UnrecoverableError();
public abstract bool RedirectToErrorPage { get; }
}
public class UnrecoverableError : ResponseStatus
{
public override bool RedirectToErrorPage
{
get { return true; }
}
}
public class TransactionSuccessful : ResponseStatus
{
public override bool RedirectToErrorPage
{
get { return false; }
}
}
When you have more response statuses then I suppose there does become a bit more duplication but it's traded off against the improved ease of use/reading that we get.
It's generally considered good practice to favour composition over inheritance and from what I can tell the virtual keyword is only ever going to be useful if you're creating an inheritance hierarchy.
An interesting lesson learned.
(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)





Comments
Dave Bush replied on Thu, 2009/05/07 - 8:30am
I disagree. Using virtual for the default implementation was actually the right way to do this. That's why it exist. You use abstract when you don't know what the implementation is suppose to be and you want to force the child classes to implement the behavior.
The problem was that you didn't pay attention to the virtual keyword which is your first clue that it may be overridden in a child class. Or maybe you relied on the IDE to tell you more than it was capable of.
I will grant you that debugging object oriented code takes a bit more work. But this is what debuggers are for. By stepping through the code you are trying to debug, it becomes extremely obvious that the method has been overridden.