Avoid calling Virtual Methods in Ctor: Tip

Yesterday I was reviewing some code at work and while doing so, I found out that there were some usages of virtual method invocation inside ctor either in the base class or derived classes. Although logically it sounds cool and even the compiler will not issue any warning, it will cause a serious problem in your code later on, may be when other developers handle your code at later point of time.

If there are bugs creeping inside this code chunk, then it is very hard to pin point or track it even though the code for visibility's sake looks great and flawless. Let me show you a sample code which depicts what I am actually talking about:

class MyBase
{
protected int someValue;
public MyBase(int value)
{
someValue = value;
MyMethod();
}
public virtual void MyMethod()
{
//Some other code goes on here..
}
}
class MyDerived : MyBase
{
string someName;
public MyDerived(int someValue) : base(someValue)
{
//Some code here
someName = string.Empty;
}
public override void MyMethod()
{
Console.WriteLine(someValue.ToString() + someName.Length);
}
}

Logically, at later point of time, if you are looking at this code, you may not notice a hidden flaw. Actually this code throws an exception. The reason is very simple and it relates to constructor sequence invocation in .NET. In the above code, before even your sub-class object construction completes, the base class constructor will call a virtual method. In the above case, it will call the sub-class overridden method, which is obvious as per OOPS.

As you can see in the overridden method, we are accessing a sub-class member, i.e., someName in the print statement. Although you may think that you have initialized a value to the someValue variable in its ctor, why is it throwing a NullReferenceException in this line? Before the constructor is fully executed, the base class is invoking this method, and the value for someName is not yet set. Hence you get an exception at run time. Even though you may think this small flaw can easily be tracked, when you are building huge apps, things some times slip off easily.

As in my case, from yesterday's code review, I found a similar code pattern usage. In yesterday's case, the developer had implemented a virtual method in the base class and had not overridden the same in derived classes, and also had called a base class virtual method from every sub-class ctor. Although it is not as harmful as the above code I showed you, it’s still a bad design. Why? Because let's say tomorrow the base class source code is not available for a future developer who is handling the same code, to fix a bug or to add a feature, he might override the virtual method. Then instead of fixing a bug, a new bug has been introduced. The below code shows the same:

class MyBase
{
protected int someValue;
public MyBase(int value)
{
someValue = value;
}
public virtual void MyMethod()
{
//Some other code goes on here..
}
}
class MyDerived : MyBase
{
public MyDerived(int someValue) : base(someValue)
{
//Some code here
MyMethod();
}
}

To solve this design problem, I started to think and then came up with a few possibilities:

1. Converting virtual method to non virtual: Very simple, but implementing it makes the class closed as per SOLID principle.
2. Using shadowing: Though sounds good to use in sub-classes, it still will not solve the problem here. So knocked off. Yes I have no idea why it did pop in my mind.
3. Explicitly make the virtual method invocation as base.MyMethod() in sub-class ctor: Could be done, but tomorrow some other developer might get confused and so it’s not a good design.

So before even selecting any solutions above, I analyzed the code further and a couple of other sub-class implementation details. I understood that this virtual method should not actually be a virtual method. Because, this method does an important job of preparing data. That means before even the sub-class starts to do some operations, they need to have this data prepared first. So it looks like option 1 was a good choice here to do, although I did not have many sub-classes overriding this virtual method. So it was an easy change. Yes, you may think that by making these changes in the base class, I have violated the Open-Closed principle, but actually I have not. And as said, as per my analysis, the base class still adheres to the Open-Closed principle pretty much with other members and as per the design, this method has to be a mandatory call and need not be customized by sub-classes.

I am still wondering how else we can solve this mistake in an enterprise application. In my case, the amount of changes were minimal, but what if it was huge?

Hope you liked my findings. Your kind comments/votes are welcome.

Thanks