2

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.

4 Answers 4

2

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).

2
  • "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" that sounds like a rather dubious claim to me. Mar 8, 2018 at 20:36
  • @whatsisname Clearly you disagree. But I'm curious as to why? (Perhaps I'm just not seeing something, so I'd like to know why you think that.)
    – Becuzz
    Mar 8, 2018 at 21:32
1

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.

1
  • I did upvote this but I don't have enough reputation yet
    – Doug Tait
    Mar 8, 2018 at 18:02
1

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.

1

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.