OOP - How to identify inheritance abusage?

Question

In looking for a simple heuristic to see when inheritance can be abused, I came up with the following hypothesis:

If subclass B overrides method foo, but does not call base.foo(), it seems like inheritance in that method is broken, as it only uses the same method signature, but not necessarily the same behavior.

Is this a right way to identify an inheritance that shouldn’t be?

Answer 1

There is a million ways to get inheritance wrong even unintentionally. There is no exhaustive list of things you can check to make sure a subclass behaves well. Even if there was, you can't really force subclasses to do it. And even if you could, the superclass can also change in ways that unintentionally break some subclasses.

A better question is, how to avoid issues, to which there are following answers:

• Avoid inheritance in the first place. No, seriously. Not only prefer "composition", but regard inheritance as a bad option.
• If you absolutely must inherit, make sure:
  • Superclass is explicitly marked abstract (written explicitly for subclassing)
  • All methods that can be overriden should be overridden, i.e. have no body.
  • All methods that should not be overridden, must not be allowed to be overwritten
  • Do not share any state, properties, methods, utility methods, etc with subclass. Regard superclass as separate object, and subclasses as completely alien objects, with the same protections (encapsulation) as you would other objects.

Even with these rules, there are still issues, like equality.

Answer 2

Robert Brautigan's answer is good and I'm not trying to supplant it, but I did want to add some direct feedback about the reasoning you posted.

If subclass B overrides method foo, but does not call base.foo(), it seems like inheritance in that method is broken

If that were the case, then it wouldn't make sense for the compiler to not just outirght enforce that base.foo() gets called (similar to how it can enforce that you have to set an out parameter somewhere in the method body).

Whether or not you call base.foo() depends on whether your derived class changes the implementation (don't call it) or further extends it (do call it). Is your derived class doing something different, or is it doing something extra?

Both of these options can be valid inheritance use cases. The hypothesis you came up with is overly strict in that it only cares about extending behavior, but not overriding it. Take a hint with the naming of the override keyword - overriding behavior is definitely a valid inheritance use case.

as it only uses the same method signature, but not necessarily the same behavior.

Your hypothesis is wrong, but I see where you're coming from and there's some merit to the underlying idea, but not the conclusion you're drawing from it.

Yes, the behavior needs to remain the same on a functional level (i.e. the method's purpose). If your base method logs a message and the overridden method deletes a database table, that's not a reasonable implementation of an override as they are two completely different behaviors.

However, that doesn't mean that the overridden method can't be different on a technical level (i.e. the code itself), where your derived method uses such a different approach (with the same intention!) that it can't really reuse the base method.

If you have never called a base method whenever you override it in any of the classes you've ever written, you're probably doing something wrong when you implement inheritance; but that doesn't mean that not calling a base method once is direct proof of inheritance abuse in that particular instance.

---

Considering what lead you to the hypothesis that you came up with, I suspect you're in a situation where you want to guarantee that your base logic gets called and that your derived classes aren't able to circumvent it.

This can be enforced with the appropriate structure in your base class, e.g.:

public class BasePizza
{
    sealed public void MakePizza()
    {
        MakePizzaBase();
        AddToppings();
    }

    private void MakePizzaBase()
    {
        // this will ALWAYS be done and derived classes cannot avoid it

        Console.Writeline("Making dough");
        Console.Writeline("Adding pizza sauce");
    }

    protected virtual void AddToppings()
    {
        // Derived classes CAN rewrite this if they want

        Console.WriteLine("Adding cheese");
    }
}

Every derived pizza you make will always have dough and pizza sauce, but they are free to avoid adding the default toppings (in this case cheese) if they want. For example:

public class LactoseFreePizza : BasePizza // Basic pizza with extra meat but NO cheese!
{
    protected override void AddToppings()
    {
        // No cheese on this!

        Console.WriteLine("Adding lots of meat");
    }
}

public class MeatLoversPizza : BasePizza // Basic pizza with extra meat
{
    protected override void AddToppings()
    {
        // Here, we do use the default toppings
        base.AddToppings();

        Console.WriteLine("Adding lots of meat");
    }
}

Notice how these methods are private and protected. The only public method is MakePizza(), so consumers must go through that methods to have their pizza made. This means that the base class is able to enforce that some logic is always executed. This relies on a few different things:

• MakePizzaBase is private so that derived classes cannot access or change it.
• MakePizza also cannot be changed by derived classes, but it needs to be public since consumers must access it. Therefore, MakePizza is sealed so that derived classes cannot just decide to change how it works.
• AddToppings is a virtual method because this is the part (of the MakePizza logic) that derived classes are allowed to change as they see fit. However, since consumers should not be allowed to call this method directly (since we want to force them to use MakePizza to ensure our base logic gets executed), we've set the AddToppings method to protected.
• Note that it would have also been possible to make AddToppings an abstract method without a method body. This means that there is no default behavior, but that you force your derived classes to explicitly define which toppings they're going to add. They can still implement an empty method, but they are forced to implement it. This makes sense in some but not all cases.

This gets you what you want. Your base class can enforce that some of its logic will always be executed, no matter what a derived class tries to do.

Note
sealed is actually optional in this example, as any method that is not virtual is inherently considered to be sealed. However, if there were a third inheritance layer and you want to inherit this method between A and B, but not towards any derivation of B (C, D or E), then A would define its method as virtual and B would inherit it while also making it sealed to ensure it doesn't get overridden in further derivations.
I prefer adding the sealed by default in this case because it telegraphs the intention, it signals to developers that this method mustn't be made virtual at any point in the future.

Note
That doesn't mean that inheritance is the right approach every time. There are plenty of cases where inheritance is not the right tool to use. This answer doesn't delve into assessing whether inheritance is right for your particular use case (since you didn't supply any concrete context), it only addresses how to implement inheritance cleanly, under the assumption that inheritance is warranted here.