Virtual Member in Constructor, workaround

Question

I have a class, BaseEmailTemplate, that formats an email, and I want to create a derived type that can overrule the defaults. Originally my base constructor -

public BaseEmailTemplate(Topic topic)
        {

                CreateAddresses(topic);
                CreateSubject(topic);
                CreateBody(topic);

        }

... (Body/Addresses)

protected virtual void CreateSubject(Topic topic)
    {
        Subject = string.Format("Base boring format: {0}", topic.Name);
    }

And in my derived

public NewEmailTemplate(Topic topic) : Base (topic)
        {

            //Do other things
        }

protected override void CreateSubject(Topic topic)
        {
            Subject = string.Format("New Topic: {0} - {1})", topic.Id, topic.Name);
        }

Of course this leads to the error discussed here: Virtual member call in a constructor

So to be absolutely blunt about this - I don't want to have to call the same methods in every derived type. On the flip side, I need to be able to change any/all. I know another base has a different subset of addresses, but the body and subject will be the default.

All three methods must be called, and the ability to alter any one of them needs to be available on a per derived basis.

I mean the thing everyone seems to be saying is an unintended consequence of using virtual seems to be my exact intention.. Or maybe I'm in too deep and singly focused?

UPDATE- Clarification

I understand why virtual members in the constructor is bad, I appreciate the answers on that topic, though my question isn't "Why is this bad?" its "Ok this is bad, but I can't see what better serves my need, so what do I do?"

This is how it is currently implemented

 private void SendNewTopic(TopicDTO topicDto)
        {
            Topic topic = Mapper.Map<TopicDTO , Topic>(topicDto);
            var newEmail = new NewEmailTemplate(topic);
            SendEmail(newEmail);  //Preexisting Template Reader infrastructure

            //Logging.....
        }

I'm dealing with a child and grandchild. Where I came in there was only newemailtemplate, but I have 4 other tempaltes I now have to build, but 90% of the code is reusable. Thats why I opted to create BaseEmailTemplate(Topic topic). BaseTemplate creates things like Subject and List and other things that SendEmail is expecting to read.

  NewEmailTemplate(Topic topic): BaseEmailTemplate(Topic topic): BaseTemplate, IEmailTempate

I would prefer not have to require anyone who follows my work have to know that

 var newEmail = new NewEmailTemplate();
 newEmail.Init(topic);

is required every single time it is used. The object would be unusable without it. I thought there were many warnings about that?

Answer 1

[10.11] of the C# Specification tells us that object constructors run in order from the base class first, to the most inherited class last. Whereas [10.6.3] of the specification tells us that it is the most derived implementation of a virtual member which is executed at run-time.

What this means is that you may receive a Null Reference Exception when attempting to run a derived method from the base object constructor if it accesses items that are initialized by the derived class, as the derived object has not had it's constructor run yet.

Effectively, the Base method's constructor runs [10.11] and tries to reference the derived method CreateSubject() before the constructor is finished and the derived constructor can be run, making the method questionable.

As has been mentioned, in this case, the derived method seems to only rely upon items passed as parameters, and may well run without issue.

Note that this is a warning, and is not an error per se, but an indication that an error could occur at runtime.

This would not be a problem if the methods were called from any other context except for the base class constructor.

Answer 2

A factory method and an initialize function is an effective workaround for this situation.

In the base class:

private EmailTemplate()
{
   // private constructor to force the factory method to create the object
}

public static EmailTemplate CreateBaseTemplate(Topic topic)
{
    return (new BaseEmailTemplate()).Initialize(topic);
}

protected EmailTemplate Initialize(Topic topic)
{
   // ...call virtual functions here
   return this;
}

And in the derived class:

public static EmailTemplate CreateDerivedTemplate(Topic topic)
{
    // You do have to copy/paste this initialize logic here, I'm afraid.
    return (new DerivedEmailTemplate()).Initialize(topic);
}

protected override CreateSubject...

The only exposed method to create an object will be through the factory method, so you don't have to worry about the end user forgetting to call an initialize. It's not quite as straight-forward to extend, when you want to create further derived classes, but the objects themselves should be quite usable.

Answer 3

A workaround could be to use your constructor to initialize a private readonly Topic _topic field, and then to move the three method calls to a protected void Initialize() method which your derived types can safely call in their constructor, since when that call occurs the base constructor has already executed.

The fishy part is that a derived type needs to remember to make that Initialize() call.

Answer 4

@Tanzelax: That looks ok, except that Initialize always returns EmailTemplate. So the static factory method won't be quite as sleak:

public static DerivedEmailTemplate CreateDerivedTemplate(Topic topic)
{
    // You do have to copy/paste this initialize logic here, I'm afraid.
    var result = new DerivedEmailTemplate();
    result.Initialize(topic);
    return result;
}

Answer 5

This answer is mostly for completeness, in case somebody stumbles upon this question these days (like me).

To avoid a separate Init method while still keeping things simple, one thing that could feel more natural (IMO) to users of the code would be to have Topic as a property of the base class:

// This:
var newEmail = new NewEmailTemplate { Topic = topic };

// Instead of this:
var newEmail = new NewEmailTemplate();
newEmail.Init(topic);

Then, the property setter could take care of calling the abstract methods, such as:

public abstract class BaseEmailTemplate
{
    // No need for even a constructor

    private Topic topic;

    public Topic
    {
        get => topic;
        set
        {
            if (topic == value)
            {
                return;
            }

            topic = value;

            // Derived methods could also access the topic
            // as this.Topic instead of as an argument
            CreateAddresses(topic);
            CreateSubject(topic);
            CreateBody(topic);
        }
    }

    protected abstract void CreateAddresses(Topic topic);

    protected abstract void CreateSubject(Topic topic);

    protected abstract void CreateBody(Topic topic);
}

Pros:

• An email template can be defined in a single line with an intuitive syntax
• No factory methods or third classes involved
• Derived classes only need to worry about overriding the abstract methods, and not about calling the base constructor (but you may still want to pass other variables as constructor arguments)

Cons:

• You still need to consider the possibility of users forgetting to define Topic, and handle the case of it being null. But I would argue you should do that anyway; somebody could explicitly pass a null topic to the original constructor
• You are publicly exposing a Topic property without really needing to. Perhaps you intended to do this anyway but, if not, it might not be very ideal. You could remove the getter, but that might look a bit odd
• If you have more than one inter-dependent property, the boilerplate code would increase. You could try to group all these into a single class, so that only one setter still triggers the abstract methods