Solving the 'Virtual method call in constructor' issue

Question

I am making a software in c#. I am using an abstract class, Instruction, that has these bits of code:

protected Instruction(InstructionSet instructionSet, ExpressionElement newArgument,
    bool newDoesUseArgument, int newDefaultArgument, int newCostInBytes, bool newDoesUseRealInstruction) {

    //Some stuff

    if (DoesUseRealInstruction) {
        //The warning appears here.
        RealInstruction = GetRealInstruction(instructionSet, Argument);
    }
}

and

public virtual Instruction GetRealInstruction(InstructionSet instructionSet, ExpressionElement argument) {
    throw new NotImplementedException("Real instruction not implemented. Instruction type: " + GetType());
}

So Resharper tells me that at the marked line I am 'calling a virtual method in constructor' and that this is bad. I understand the thing about the order in which the constructors are called. All overrides of the GetRealInstruction method look like this:

public override Instruction GetRealInstruction(InstructionSet instructionSet, ExpressionElement argument) {
    return new GoInstruction(instructionSet, argument);
}

So they don't depend on any data in the class; they just return something that depends on the derived type. (so the constructor order doesn't affect them).

So, should I ignore it? I'd rather not; so could anyone show me how could I avoid this warning?

I cannot use delegates neatly because the GetRealInstruction method has one more overload.

Answer 1

I've encountered this issue quite a few times and the best way I've found to properly resolve it is to abstract the virtual method that is being called from the constructor, into a separate class. You would then pass an instance of this new class into the constructor of your original abstract class, with each derived class passing it's own version to the base constructor. It's a bit tricky to explain so I'll give an example, based on yours.

public abstract class Instruction
{
    protected Instruction(InstructionSet instructionSet, ExpressionElement argument, RealInstructionGetter realInstructionGetter)
    {
        if (realInstructionGetter != null)
        {
            RealInstruction = realInstructionGetter.GetRealInstruction(instructionSet, argument);
        }
    }

    public Instruction RealInstruction { get; set; }

    // Abstracted what used to be the virtual method, into it's own class that itself can be inherited from.
    // When doing this I often make them inner/nested classes as they're not usually relevant to any other classes.
    // There's nothing stopping you from making this a standalone class of it's own though.
    protected abstract class RealInstructionGetter
    {
        public abstract Instruction GetRealInstruction(InstructionSet instructionSet, ExpressionElement argument);
    }
}

// A sample derived Instruction class
public class FooInstruction : Instruction
{
    // Passes a concrete instance of a RealInstructorGetter class
    public FooInstruction(InstructionSet instructionSet, ExpressionElement argument) 
        : base(instructionSet, argument, new FooInstructionGetter())
    {
    }

    // Inherits from the nested base class we created above.
    private class FooInstructionGetter : RealInstructionGetter
    {
        public override Instruction GetRealInstruction(InstructionSet instructionSet, ExpressionElement argument)
        {
            // Returns a specific real instruction
            return new FooRealInstuction(instructionSet, argument);
        }
    }
}

// Another sample derived Instruction classs showing how you effictively "override" the RealInstruction that is passed to the base class.
public class BarInstruction : Instruction
{
    public BarInstruction(InstructionSet instructionSet, ExpressionElement argument)
        : base(instructionSet, argument, new BarInstructionGetter())
    {
    }

    private class BarInstructionGetter : RealInstructionGetter
    {
        public override Instruction GetRealInstruction(InstructionSet instructionSet, ExpressionElement argument)
        {
            // We return a different real instruction this time.
            return new BarRealInstuction(instructionSet, argument);
        }
    }
}

In your particular example it does get a little confusing and I started to run out of sensible names, but this is due to the fact you already have a nesting of Instructions within Instructions, i.e. an Instruction has a RealInstruction (or at least optionally does); but as you can see it is still possible to achieve what you want and avoid any virtual member calls from a constructor.

In case that's still not clear, I'll also give an example based on one I used just recently in my own code. In this case I have 2 types of forms, a header form and a message form, both of which inherit from a base form. All forms have fields but each form type has a different mechanism for constructing its fields, so I originally had an abstract method called GetOrderedFields that I called from the base constructor and the method was overriden in each derived form class. This gave me the resharper warning you mention. My solution was the same pattern as above and is as follows

internal abstract class FormInfo
{
    private readonly TmwFormFieldInfo[] _orderedFields;

    protected FormInfo(OrderedFieldReader fieldReader)
    {
        _orderedFields = fieldReader.GetOrderedFields(formType);
    }

    protected abstract class OrderedFieldReader
    {
        public abstract TmwFormFieldInfo[] GetOrderedFields(Type formType);
    }
}

internal sealed class HeaderFormInfo : FormInfo
{
    public HeaderFormInfo()
        : base(new OrderedHeaderFieldReader())
    {
    }

    private sealed class OrderedHeaderFieldReader : OrderedFieldReader
    {
        public override TmwFormFieldInfo[] GetOrderedFields(Type formType)
        {
            // Return the header fields
        }
    }
}

internal class MessageFormInfo : FormInfo
{
    public MessageFormInfo()
        : base(new OrderedMessageFieldReader())
    {
    }

    private sealed class OrderedMessageFieldReader : OrderedFieldReader
    {
        public override TmwFormFieldInfo[] GetOrderedFields(Type formType)
        {
            // Return the message fields
        }
    }
}

Answer 2

When you create an instance of your derived class, your call stack will look like this:

GetRealInstruction()
BaseContructor()
DerivedConstructor()

GetRealInstruction is overridden in the derived class, whose constructor has not finished running yet.

I don't know how your other code looks, but you should first check if you really needed a member variable in this case. You have a method that returns the object you need. If you really do need it, make a property and call GetRealInstruction() in the getter.

Also you can make GetRealInstruction abstract. That way you don't have to throw the exception and compiler will give you an error if you forget to override it in a derived class.

Answer 3

You can introduce another abstract class RealInstructionBase so your code will look like:

public abstract class Instruction {
   public Instruction() {
       // do common stuff
   }
}

public abstract class RealInstructionBase : Instruction {
   public RealInstructionBase() : base() {
       GetRealInstruction();
   }

   protected abstract object GetRealInstruction();
}

Now each instruction which needs to use RealInstruction derives from RealInstructionBase and all others derive from Instruction. This way you should have them all correctly initialized.

EDIT: Ok, this will only give you a cleaner design (no if in constructor) but does not get rid of the warning. Now if you would like to know why you get the warning in the first place, you can refer to this question. Basically the point is that you will be safe when you mark your classes where you are implementing the abstract method as sealed.

Answer 4

You could pass in the real instruction into the base class constructor:

protected Instruction(..., Instruction realInstruction)
{
    //Some stuff

    if (DoesUseRealInstruction) {
        RealInstruction = realInstruction;
    }
}

public DerivedInstruction(...)
    : base(..., GetRealInstruction(...))
{
}

Or, if you really want to call a virtual function from your constructor (which I highly discorage you from) you could suppress the ReSharper warning:

// ReSharper disable DoNotCallOverridableMethodsInConstructor
    RealInstruction = GetRealInstruction(instructionSet, Argument);
// ReSharper restore DoNotCallOverridableMethodsInConstructor

Answer 5

Another option is to introduce an Initialize() method, where you do all the initialization that requires a fully-constructed object.