Should a base class implement a virtual method for the most common type or derived class?

Question

Is it okay to implement a virtual method in a base class because I know that the majority of derived classes will use that particular implementation?

Suppose I have the following class hierarchy (in C#):

public abstract class BaseClass {
    public virtual void MethodA() {
        //Implement MethodA as DerivedClassA and DerivedClassB
        //need it to avoid code duplication for
        //DerivedClassA.MethodA and DerivedClassB.MethodB
    }
}

public class DerivedClassA : BaseClass {}

public class DerivedClassB : BaseClass {}

public class DerivedClassC : BaseClass {
    public override void MethodA() {
        //Do MethodA in a DerivedClassC kinda way
    }
}

I feel like it should be okay because this seems to be the reason that virtual methods exist but something also feels a little "smelly" about it to me. I guess I would just like confirmation that this approach is okay or to be told why its bad.

Answer 1

I would only implement the method in the base class if it is an acceptable default for all subclasses (both currently known and future).

A virtual method is a promise of being able to perform some kind of behavior. Overriding that behavior is saying that your subclass requires more processing, more steps, etc. in order to be able to fulfill that contract. As such, one thing I was always taught was that if you have a virtual method and aren't calling your parent's version of the same method, you probably have your inheritance hierarchy all screwy (a code smell, not by definition wrong, but something probably isn't right). If your class doesn't do something like its parent, does it really deserve to be a child class of that parent? Or should it be more like a sibling?

I would make your classes more like this:

public abstract class BaseClass {
    public abstract void MethodA();
}

public abstract class DerivedClassThatDoesThingsTheFooWay : BaseClass {
    public virtual void MethodA() {
        //Implement MethodA as DerivedClassA and DerivedClassB
        //need it to avoid code duplication for
        //DerivedClassA.MethodA and DerivedClassB.MethodB
    }
}

public class DerivedClassA : DerivedClassThatDoesThingsTheFooWay {}

public class DerivedClassB : DerivedClassThatDoesThingsTheFooWay {}

public class DerivedClassC : BaseClass {
    public override void MethodA() {
        //Do MethodA in a DerivedClassC kinda way
    }
}

This way you move common functionality into a common place that doesn't make your other classes depend on or have irrelevant behavior. If there is a common and acceptable default, the base class is fine. If that functionality would be completely wrong in some cases, move it into a class somewhere in the middle (or use composition instead of inheritance).

Answer 2

Seems fine to me in principle, but it depends exactly what code is shared, why it is being shared, and how this might change if you add a DerivedClassD, DerivedClassE, etc.

If you have an implementation of MethodA that you generally expect to be shared by all or most implementations of the derived classes, then it makes sense to pull this up into the parent class, then override it in the few occasions where there are exceptions.

But if the code just happens to be shared by two of the derived classes, but is not necessarily going to be shared by any more that might be added, then it might make more sense to do it slightly differently, for example introduce an intermediate class which inherits from BaseClass and have DerivedClassA and DerivedClassB inherit from this intermediate class while DeriveClassC still inherits directly from BaseClass.

Answer 3

If the methods pertain to the real reason the class exists, your approach is fine. If the methods are just utility functions and inheritance is being used as a means to share those utilities, you should favor Composition over Inheritance.

Good

public class DiscountCalculator
{
    public virtual decimal CalculateDiscount(decimal price)
    {
        return price * 0.1m;
    }

    // other methods
}

public class NewMemberDiscountCalculator : DiscountCalculator
{
    // other methods
}

public class PreferredDiscountCalculator : DiscountCalculator
{
    public override decimal  CalculateDiscount(decimal price)
    {
        return price * 0.25m;
    }

    // other methods
}

Why this is ok: These classes calculate discounts. The child class still calculates a discount, but is a more specialized version.

Bad

public class DiscountCalculator
{
    public virtual string FormatDiscount(decimal price)
    {
        return CalculateDiscount(price).ToString("C");
    }

    // other methods
}

public class NewMemberDiscountCalculator : DiscountCalculator
{
    // other methods
}

public class PreferredDiscountCalculator : DiscountCalculator
{
    public override string FormatDiscount(decimal price)
    {
        base.FormatDiscount(price) + "!!!";
    }

    // other methods
}

Why this is bad: These methods are utility methods that format the discount. This class shouldn't be formatting the discount, since its purpose is to calculate discounts.

Answer 4

Yes, of course. That is exactly the point of abstract classes. An abstract class allows you to define some 'default' behavior for all subclasses. The alternative, with no default behavior, is an interface.

Marking abstract class functions as abstract also lets you signal where subclasses must define those functions, rather than just virtual where it is optional. It's often common to have non-virtual functions in the base class, which call abstract functions that subclasses are required to define.

It's not a code smell. It's using the language features in exactly the way they were meant to work.