Is it logical to not use inheritance because the function is critical?

Question

Our codebase has a typical base-class with a ton of sub-classes. The base-class already has many default functions for the sub-classes.

However, one particular function has the same verbatim implementation in almost all of the sub-classes. (Each of the rare cases has its own unique implementation.) The rationale I hear is that it's important for that function to be correct, so we want to ensure that the developer of a new sub-class goes through the steps to make sure they have the right logic. In other words, they don't blindly inherit the function and miss the case of the need to overload the function to handle special cases.

Is this a good argument at the cost of code duplication, code complexity, review time, repository size, etc? Developers often just copy code from other sub-classes, but supposedly having to commit explicit code might be useful.

I believe inheritance and a well-documented development and review process should suffice.

Edit

I totally buy in composition if the function implementation has decent variance. Since our implementation is verbatim the same for, say, 98 out of 100 sub-classes, I wasn't sure if it's fair to require the 98 to compose instead of inherit by default. I went over strategy and factory in my mind before posting, but thought inheritance more sensible?

I am just very happy that the consensus is to use design better than duplication, my original main concern.

Answer 1

This is a “protect the idiots” rule. I’ve seen it in many forms. Once entrenched it’s very difficult to overturn. I’ve seen rules like this forbid code that is identical to code in the language specification. I’ve seen using the return value of the assignment operator forbidden. I’ve seen while loops forced to have unnecessary bodies. I’ve seen every if forced to have an else. I’ve never seen these silly rules help.

What I’ve seen help is checking the readability of the code by asking someone, who didn’t write it, to read it. If you want idiot proof code hire an idiot to review it. Don’t make up style guide rules that cripple the language. Find better ways to express the code if they exist. Use them when they make sense. Not because some rules weenie who isn’t in the room said so.

And yes, this goes for linters as well. Don’t tell me to respect what it has to say if you can’t explain why. God gave you a brain. Use it.

Answer 2

This is difficult to tell without analyzing the code. I can't provide you an authoritative answer, but perhaps some guidance on how to make this decision will do.

In a situation like this, the first thing I would do is diff the code in each of the methods. A general purpose diff tool like KDiff3 allows you to paste code into two different panels for an ad hoc comparison.

If you don't notice any differences across all of the sub classes — that is, the are absolutely no differences — then I would agree that this is paranoia. Kilian's comment is probably spot on. Some time ago some nasty bug surfaced in that code and it was a big deal. Now developers are told to copy and paste without touching it to appease someone. I would say this is the least likely case.

If you notice slight differences from one class to another, then this copied and pasted code likely represents a lot of hard-won lessons. Don't be fooled by similar looking code. It's the little differences that make all the difference. Even if 90% of the code is the same, the 10% that is different can justify code duplication.

Don't get to hung up on the DRY principle here. Eliminating code duplication is not the goal. Most principles, architectures, and nebulous "best practices" aim to reduce bugs, make code easier to test, and enable code to evolve without requiring every use case to be retested. This requires copying and pasting code more often than you think.

DRYing up your code can introduce coupling between components and use cases that shouldn't know about each other. A subtle shift in the logic needed for one sub class could wreak havoc on other sub classes, which would require retesting parts of the application that shouldn't need to be tested. This might be the reason for the rampant copying and pasting.

The trick with eliminating code duplication is to identify a good abstraction and give it a good name. The abstraction should simplify the consuming code and make it easier to understand. If it doesn't, then consider renaming things until consumers of the abstraction are simpler, or admit it isn't helping, and just copy and paste the code. If you do create a new class, make it testable, and actually test it.

If analyzing code isn't giving you any satisfaction, consult version control. Look at the history of these classes, especially the old ones and the base class. I bet if there was some nasty bug a while back you'll notice a commit or two about it. With any luck that commit will have a person's name, and the id of a work item in it. If the author of that commit still works there, send them an email, instant message, or walk up to them and ask. Try looking up the work item to read through the comments. Failing that, find the person who has been there the longest, perhaps even someone from production support, and ask them. Production support folks tend to remember the really nasty bugs.

Unfortunately, strangers on the Internet can't answer this for you. The best you can do is go on an archeological dig through the code and version control, and ask people.

I suspect you'll find differences between these methods across sub classes, but not consistent enough differences to pull this logic into its own class. That's just my intuition speaking, though. If this is the case, inheritance introduces coupling between components. This implies the logic for all derived classes must evolve at the same time and for the same reasons. If this is not true, then eschewing inheritance in favor of copying and pasting makes sense, even if it is annoying.

Answer 3

it's important for that function to be correct, so we want to ensure that the developer of a new sub-class goes through the steps to make sure they have the right logic.

I would question the core premise of this argument, in that it is very easy for developers to cut and paste an implementation from another class without giving it much thought and you are now left with multiple implementations that all have to be updated should an issue be discovered. More insidiously it is also possible that one of the implementations has a minor functional difference and without careful reading of each it's not going to be obvious that the change has been made.

In short you haven't achieved the core "protection from idiots" goal that the strategy is intended to achieve.

---

An "OOP solution" to the same problem would be to introduce an intermediate class that extends from the base and provides the desired function - the classes that need this function can now inherit from the intermediate class. The obvious flaw with this is that if you are coding in a language with single inheritance you will be restricted in the number of places you can use the strategy.

That said, I would consider using this strategy in a couple of cases, specifically:

• If there is a good name for the intermediate class (e.g.: FileProcessor) where it's clear that the subclasses all have an obvious relationship to the intermediate class (XmlFileProcessor, JsonFileProcessor, ...).
• Earlier on in the development, of a new piece of code - it would force me/others to think about exactly which class should be inherited from. Later on, once the structure of the code has become more stable it's a quick refactor to move the method up and eliminate the intermediate class.

---

Finally I would consider composition - moving the actual implementation out of the inheritance tree - this has the advantage that classes that need the method can use a few boilerplate lines, just to forward the call onto the actual implementation.

Answer 4

As far as I can see this problem will not automatically be solved by switching from inheritance to composition. When I understand this correctly, in your system, each new subclass (or new module of a certain category) requires a specific functionality. In 80% of all cases a standard implementation of this functionality is sufficient, in 20% it is not.

Now, your team fears providing the standard implementation in form of a reusable function, because it makes it too easy to leave out the assessment whether the module belongs to the former "80%" or "20%".

This problem does not change, regardless if the reusable function is provided by inheritance from a base class, or if the reusable function is provided through a separate helper class.

So if switching to composition alone does not solve the issue, what helps? Usually the answer is, the correct combination of both. I don't know your code, but in certain cases the following thing helps:

• start with an abstract virtual function in your base class. Add a clear note in the code how this function has to be implemented and that for certain special cases a "default implementation" might not be sufficient. Refer to examples in your code base.

• provide the default functionality as part of a reusable function in a helper class, and document what it handles, and what not, care for naming and tests.

• now, when someone creates a new subclass, they have at least think twice how to implement the abstract function

• the default functionality from the helper class might be called in the 80% of the implementations of the abstract function. This leads to repetition of some delegational code, but the logic stays in a single place

• find ways to implement the missing 20% - ideally not by copy-pasting the default functionality from the helper class and modifying it, but by providing more reusable building blocks in the helper class.

At the and of the day, nothing will stop unexperienced or sloppy devs from blindly copy-pasting code and programming by coincidence. Still providing mature, tested, reusable functionality to those developers in a way they can rely on it should usually be a way to reduce errors and increase maintainability - forcing people however to copy-paste code will often lead to the opposite.

Answer 5

“Base class with a ton of sub-classes” is not typical… and many functions in the base class is even more troublesome.

Once you’re at that point, it is somewhat reasonable to fear that engineers will overlook one of the functions when doing inheritance. Depending on documentation and code reviews isn’t exactly a great plan either. Documentation gets out of date quickly and code review is filled with human error.

Finding some programmatic way for engineers to “fall into success” is ideal.

Answer 6

You have a management/developer trust issue, not an implementation issue.

The fundamental problem is that your organization doesn’t trust that a developer will do the right thing, where the right thing is analysis and generally results in no action.

Given that, the less code changed, the less confidence they will have that the right thing was done.

Fortunately, there is a way to provide both the extra code they want and the nudge that your developers may need to ensure they do the analysis. Unit tests. You can use inheritance with a virtual method that can be overridden with a different implementation when necessary, and use unit tests to prove what happens is correct for the new derived class.

It doesn’t require that new code be written, but you can make that the process and enforce it when the code is reviewed, and if new unit tests are written, that is evidence that the developer did the analysis required to determine whether the default implementation is sufficient.

Answer 7

This seems like a bad idea. In an ideal world, this would not be needed, but not only idiots make mistakes.

You seem to introduce "worse" code (duplication) to solve this problem, but rely on a process to still identify the issue:

Developers often just copy code from other sub-classes, but having to commit explicit code might be useful.

Instead of developers duplicating code to double check during the commit if this is correct, why not use a process that is exactly right for this purpose: A code review and pull requests.

It would still be nice to have an alternative that does not rely on humans (always trouble with humans) and enforced by the code itself. Without knowing your exact code, I want to give a few suggestions:

• Template Method

    final void foo() {
        normalCase();
        specialCase();
    }
  
    final void normalCase() {}
    abstract void specialCase();
  

• Composition (maybe more concretely a Strategy)

    sealed interface FooCaseStrategy permits FooNormalCase, FooSpecialCase { void handle(); }
  
    class MyFoo {
        private FooCaseStrategy fooHandler;
  
        final void foo() {
            fooHandler.handle();
        }
  
    }
  

Answer 8

I think your question needs some analysis before jumping to the answers. The answers already given are far to technical and are somewhat lost in the details.

What do you want, what is it you do now to achieve that and is that actually a good solution for what you want?

Obviously you want to avoid errors in your code. Errors can be introduced through a multitude of ways, only one of them is developer stupidity. Lack of time, lack of understanding, stress, typos etc. are some more.

The solution you've outlined is to avoid a very specific error in the process of coding itself. Not by unit tests, not by integration tests, but already in the process of making the code.

This is a difficult approach because software designs already tend to be too complex. Introducing additional complexity by choosing a certain odd design pattern to avoid an only vaguely defined possible human error leads to additional complexity and an unstable design. Your idea of what a developer could do wrong will inevitably change over time and thus this will change the requirements to your design.

That being said it is always a good idea to make designs that avoid misuse as much as possible. (By the details you've given it sounds like this opportunity has already been missed.) The question is at what lengths.

In my eyes code duplication is sometimes worth the effort, but only for very special occasions like where different entities share a common understanding of a concept but are otherwise completely separated for a reason. Avoiding developer errors IMHO is absolutely no reason to use it, especially if you have no control over the multiplier. (How many times the code is actually duplicated.) It will make refactoring much more difficult in the future, it makes it harder to spot differences in the various implementations, fix bugs in it etc.

The question that comes up in my mind is why you don't use testing to detect these kinds of errors? Tests are a much better approach to prevent very specific errors from happening, also they work better with changing requirements (ie. your understanding of things that could go wrong) and they don't influence the actual software design that much.

Answer 9

The code duplication is evil and should be fixed.

We have a lot of code duplication in a bunch of test cases which all set up certain scenarios in a rather complicated test environment for performing the actual tests. In many cases this setup code (a screen full of code or two) is simply copied, most likely because the entire new test case is started as a copy of an existing test case. A few modifications in the setup are made to suit this particular case.

Now over time, differences creep in.

The test environment is improved and offers new convenience methods. Some tests need only three objects under test, not four. Formatting is changed, within the permitted spectrum.

Over time each test case looks a little different. In order to see what is different within all the noise you have to actually copy and paste that code segment into a diff program of your choice. Little changes can be essential. It becomes impossible to tell whether the differences are necessary or arbitrary. The test case becomes impossible to understand quickly. It becomes hard to tell whether the test environment is set up correctly at all.

This code redundancy is terrible and must be eliminated with extreme prejudice. The replacement is a function which performs the setup. It caters to the different needs of the test cases through parametrization. The differences in the setup between test cases can now be immediately understood from the different arguments: Start up one or two objects under test? To which state? Perform full initialization? etc.

Because the function has been thoroughly reviewed and debugged and is heavily used, we can rely on it. When it finishes, we can be sure the correct scenario has been set up. There is a separation of concerns (even though that does not apply to your use case). All errors now come from the actual test, or its object.

(I have not even mentioned the usual and valid objection against code duplication, namely that it is badly maintainable when you want to change something in the duplicated code.)

Answer 10

I argue that it is possible to reduce repeating yourself without inheritance.

Inheritance is often used in the following contexts:

1. (As an interface.) When objects must demonstrate that they have a method with the expected signature. For instance, any object that implements ICanSpeak necessarily has a method speak(str) with that exact signature:

   def say_hello(speaker: ICanSpeak):
       speaker.speak("hello")
   

2. (As a "syntactic sugar".) When there is an explicit default behavior that is nearly always the desired behavior with a probability p.

The threshold for (2) depends on the culture and conventions of those who work with the code. Is p ≥ 10% enough? 50%? 98%? 100%? Everyone's tastes are different.¹ If that threshold is not met, all is not lost. A fierce storm of complexity rages around us, but we may still keep DRY.

For instance,

def common(self, *args, **kwargs):
    ...

class BruceSpringsteen:
    def request(self, *args):
        return common(self, *args)

class BillyJoel:
    def request(self, *args):
        return common(self, *args)

class BruceDickinson:
    def request(self, *args):
        return common(self, *args, needs_more_cowbell=True)

In this example, the common function reduces some amount of repetition. The desired functionality must be explicitly provided. There are even optional feature flags should one decide to customize the common behavior.

Inheritance strips away a proportion p of the redundancy within this pattern further:

class UsualRequest:
    def request(self, *args, **kwargs):
        ...

class BruceSpringsteen(UsualRequest):
    pass

class BillyJoel(UsualRequest):
    pass

class BruceDickinson(UsualRequest):
    def request(self, *args):
        return super().request(*args, needs_more_cowbell=True)
        # usual request, but with extra cowbell

Because this is so common, language designers decided to introduce the concept of inheritance. And yet, while inheritance as in (1) is necessary as it provides a concrete role within the type system as an interface, inheritance as in (2) is merely just a common syntactical convenience to avoid calling the underlying common method.

---

^{1 Mathematically, there is absolutely nothing special about inheritance that prevents one from choosing culturally abhorrent default behavior. For instance, class Speaker: def speak(x: str): os.system("rm -rf /") is technically entirely valid default behavior that is applicable 0% of the time.}

Answer 11

I have never seen any places where inheritance have been a good way to solve what it's doing.

When the need for inheritance occurs, it's time to refractor and seperate the logic into seperate modules, so you can consume it where you need it.