Mark Needham

Thoughts on Software Development

C#: Using virtual leads to confusion?

with 7 comments

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.

Be Sociable, Share!

Written by Mark Needham

May 6th, 2009 at 7:30 pm

Posted in .NET

Tagged with ,

  • Pingback: DotNetShoutout

  • Pingback: Reflective Perspective - Chris Alcock » The Morning Brew #342

  • http://80points.blogspot.com/ Carlo

    I can’t see the composition behaviour above, it still is inheritance…

    Isn’t it also bad practice to let base classes know about their sub-classes i.e. The 2 static fields in the base class.

  • http://www.markhneedham.com Mark Needham

    @Carlo yeh it’s definitely still inheritance – that was supposed to be a more general comment about where the virtual key word might be useful.

    I’m not sure, maybe it is. The idea was to only use the objects like:

    ResponseStatus.UnrecoverableError or ResponseStatus.TransactionSuccessful so I’m not sure if it’s quite so bad or not?

  • Rob Scott

    I’m pretty sure that a better title might have been “Deriving from concrete class causes confusion”. It’s long been recognized that deriving from concrete classes is problematic. For a C++ version of the argument see Scott Meyers “More Effective C++”, Item 33.

  • Mark Brackett

    I think I’d word this argument more along the lines of “making RedirectToErrorPage = true by default is bound to cause confusion when you inherit the same class for both errors and successes”. Not anywhere as catchy, but highlights the real problem AFAIC.

    In other words – why would a ResponseStatus (1) care whether your UI redirects to an error page, and (2) default to true?

  • http://80points.blogspot.com/ Carlo

    I see where you’re going with that. My take is that as long as there isn’t lots of behaviour then the base class referencing subclasses isn’t an issue.