Virtual Hell

Today, I had the misfortune to come across some code that looked highly dodgy to me.

I’ll do the short version, as the class was so abysmal, that my brain would hurt pointing out the rest at this stage. I’ll just show you the school boy errors and wonder how they ever got through the interview.

We’ll start with a simple interface and 2 classes implementing that interface:

public interface IValidator
{
void DoSomething();
}

public class DerivedValidator : IValidator
{
public void DoSomething()
{
Console.WriteLine("Derived DoSomething");
}
}

public class AnotherDerivedValidator : IValidator
{
public void DoSomething()
{
Console.WriteLine("AnotherDerived DoSomething");
}
}

OK, simple enough.

They then created some view models to take advantage of this wonderful validation:

public class DodgyViewModelBase
{
public void Validate()
{
Validator.DoSomething();
}

protected IValidator Validator { get; set; }
}

public class DodgyDerivedViewModel : DodgyViewModelBase
{
public DodgyDerivedViewModel()
{
Validator = CreateValidator();
}

protected virtual IValidator CreateValidator()
{
return new DerivedValidator();
}
}

public class DodgyFurtherDerivedViewModel : DodgyDerivedViewModel
{
protected override IValidator CreateValidator()
{
return new AnotherDerivedValidator();
}
}

Now, I can see what the well meaning programmers were trying to do here. They were trying to create a scenario where they can change the validator based on the type of view model created.

Now, as you can tell by the name of the class, I view this as highly dodgy. So, let’s go through some of the points:

1. The base class can be instantiated. This would mean there would be no IValidator set up and a call to it would result in a null object reference exception.
2. You are passing on responsibility to create the IValidator with no enforcement for this, again resulting in point 1.
3. Because the CreateValidator is called from the constructor, then some strange things can happen. The constructors get called in a cascading fashion, starting with the base constructor. So, if the DodgyFurtherDerviedViewModel gets constructed, the first constructor called will be DodgyViewModelBase. Great, just what we expected, the default constructor (provided by the compiler) does nothing anyway, so no side effects there. The next constructor to get called is our DodgyDerivedViewModel constructor. This calls our CreateValidator virtual method. Lovely jubbly, that calls our CreateValiator method on the DodgyFurtherDerivedViewModel, just as expected. Everything’s fine, what’s my problem??? Well, suppose my DodgyFurtherDerivedViewModel constructor had to do some initialisation in order for my CreateValidator to work. Its constructor hasn’t been called yet, so anything my CreateValidator was relying on hasn’t happened yet. There’s no real way of the poor developer being aware that this is happening, remember they’ve just been told to implement the DodgyFurtherDerivedViewModel.

Let’s look at a (slightly) better version:

public abstract class BetterViewModelBase
{
protected BetterViewModelBase()
{
Validator = CreateValidator();
}

public void Validate()
{
Validator.DoSomething();
}

protected IValidator Validator { get; set; }

protected abstract IValidator CreateValidator();
}

public class BetterDerivedViewModel : BetterViewModelBase
{
protected override IValidator CreateValidator()
{
return new DerivedValidator();
}
}

public class BetterFurtherDerivedViewModel : BetterDerivedViewModel
{
protected override IValidator CreateValidator()
{
return new AnotherDerivedValidator();
}
}

Ok, with the above code, we’ve mitigated points 1 and 2. We can no longer instantiate the base class with the adverse effects it created. Also, the developer knows it’s their responsibility to implement CreateValidator. But, point 3 still remains, some weird stuff could go on in the construction and use of CreateValidator. There must be an even better way of preventing this.

I’ll now show you the final version, which should clear up all the points and make the classes a lot safer to use:

public abstract class GoodViewModelBase
{
private IValidator validator;

protected GoodViewModelBase(IValidator validator)
{
this.validator = validator;
}

public void Validate()
{
validator.DoSomething();
}
}

public class GoodDerivedViewModel : GoodViewModelBase
{
public GoodDerivedViewModel(IValidator validator)
: base(validator)
{
}
}

public class GoodFurtherDerivedViewModel : GoodDerivedViewModel
{
public GoodFurtherDerivedViewModel()
: base(new AnotherDerivedValidator())
{
}
}

Here, we inject the IValidator into the constructor, so there are no virtual methods to call to mess things up. In this particular code, the GoodFurtherDerivedViewModel would not be needed full stop, as we can inject the functionality needed without resulting in the need for inheritance (which is always a good thing). You should always strive to rid your code of inheritance (of concrete classes at least).

Try and restrict inheritance to interface inheritance, where you are providing the functionality rather than the data.

This last class has some nice side effects, a major one being testability. Particularly when we use mocking libraries. We can simply mock the IValidator in our unit tests and inject the required behaviour into the constructor.

BTW. YOU do do unit tests, don’t you?

Obviously, the developer that wrote the original code didn’t, otherwise they would have found it very hard to test and also, would’ve probably found the pitfalls long before my roving eye had the misfortune to gaze upon it.

And if you think this is contrived, don’t, this is actual code that I am looking at today in someone’s business.