C# · CodeProject · Dotnet

Avoid calling Virtual Methods in Ctor TIP

Dear Reader,

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

If there is any bug creeping inside this code chunk, then its very hard to pin point or track it even though the code for visibility sake looks great and flawless. Let me show you a sample code which depicts as 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 its 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 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 print statement. Although you may think that you have initialized a value to someValue variable in its ctor, still why its throwing a NullReferenceException in this line.Β  Since before the constructor is fully executed, the base class is invoking this method the value for someName is not yet set. Hence you get an exception at run time. Even though you may think this small flaws can easily be tracked, but when your building huge app things some times slip off easily.

As in my case from yesterdays code review, i found a similar code pattern usage. In yesterdays case the developer had implemented virtual method in base class and has not overridden the same in derived classes and also had called base class virtual method from every sub-class ctor. Although it is not as harmlful as the above code i showed you. But it’s still a bad design. Why? Because lets say tomorrow the base class source code is not available with future developer who is handling the same code and to fix a bug or to add a feature he might overrides 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();
}
}

So to solve this design problem, i started to think and then i came up with 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 but still it 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 still it’s not a good design.

So before even selecting any solutions above, i analyzed code further and couple of other sub-classes implementation details.I understood that this virtual method should not actually be a virtual method. Because this method does some important job of preparing some 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 making this changes in base class i have violated Open-Closed principle, but actually it’s not. And as said as per my analysis the base class is still adheres to 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 can you solve this mistake in an enterprise application actually. In my case the amount of changes were minimal, but what if it was huge?

Hope you liked my learning’s. Your kind comments/votes are welcome.

Thanks πŸ™‚

Advertisements

Leave a Reply

Please log in using one of these methods to post your comment:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s