Should classes with business logic inherit from a class with helper methods, or vise-versa?

Question

I have a codebase where some classes contain both "essential" business logic, and "incidental" complexity. I am considering a refactor where I leverage inheritance to improve the readability and maintainability of the code. However, it's not clear if the base class should be the one containing the non-core logic (and its child implements the core logic) or the other way around.

To illustrate my situation, consider the following class:

class MyDoEverythingClass:
    def __init__(self, ...):
        ...

    def calculate_tax(self):
        ...

    def validate_data(self):
        ...

    def persist_to_file(self):
        ...

    def plot(self):
        ...

In this case the only "real" logic here is the calculate_tax method - so I could refactor this as:

class CoreLogic:
    def __init__(self, ...):
        ...

    def calculate_tax(self):
        ...

class ExtendedFunctionality(CoreLogic):
    def validate_data(self):
        ...

    def persist_to_file(self):
        ...

    def plot(self):
        ...

or the other way around:

class NonCoreLogic:
    def __init__(self, ...):
        ...
    def validate_data(self):
        ...

    def persist_to_file(self):
        ...

    def plot(self):
        ...

class CoreLogic(NonCoreLogic):
    def calculate_tax(self):
        ...

Are there reasons to favour one over the other?

Answer 1

Avoid unnecessary inheritance. Do inherit from another class if you want to inherit its public interface – if all users of your class should also be able to use the methods and properties of the base class.

But in many cases, users of your class should not have access to these helpers. Then, don't inherit from them.

Instead, if the helpers form a meaningful class, use composition over inheritance:

class SomeHelpers:
  def step1(self):
    ...

  def step2(self):
    ...


class PublicFunctionality:
  def __init__(self):
    self._helpers = SomeHelpers()

  def do_something(self):
    return self._helpers.step1() + self._helpers.step2()

A “meaningful class” in this context means a class that either involves polymorphism or encapsulation.

In other cases, the helper functionality does not form a meaningful class. Python is a multi-paradigm language that doesn't expect you to stuff functions into a class just so that there is a class. The usual approach would be to just have a bunch of “free” functions:

def step1():
  ...

def step2():
  ...


class PublicFunctionality:
  def do_something(self):
    return step1() + step2()

Answer 2

I'm going to try and ignore the names of the functions/classes you've given - as some other comments have been made already about how it sounds like you might have a single-responsibility-refactoring to do. But I think that your intention is that this is a specific question which is unrelated to responsibility of class.

So the answer I think is the second way. But not exactly.

The key thing about inheritance is that what you're really trying to do is specify an interface, so that other application code can rely on certain functions being callable when it is handling what it (or what the author) only knows to be a base class and not the derived one. This is sort of less obvious in python because of its nature - it doesn't force you into paradigms and so it doesn't have an idea of abstract functions naturally built in. This is unlike say C++ where that's been a key concept of the language design, where you make declarations like:

virtual Result validate_data() const = 0;

or in Java where you declare an interface rather than a class. Here what you're doing is setting up a contract, then you inherit from it and implement. I mean I think you need to be careful with your terms, when you say CoreLogic and NonCoreLogic these are leading you away from thinking about the CoreInterface and the implementation of that.

The idea of a contract or interface is it is the thing that the rest of the application binds to. If your interface grows in size, in number of functions, that's another issue and is not solved by inheritance, although it may be a part of the solution. It needs to be handled by reconsidering what collection of objects are needed to represent the pipeline of tasks that need to be performed in a step by step way (with clear and single responsibilities). E.g. Is validate_data a function on some kind of Parser actually, which returns a ValidatedData object, which can calculate its own tax. To be honest, you're best off not refactoring at all until you've seen the shape of one of those little task pipelines, those responsibilities, and can pull one out into a new set of classes.

Anyway. If you are doing inheritance and adopt the first way you mentioned then you're kind of doing something which is better done by composition rather than inheritance as the other posts/comments have pointed out. With inheritance you can specify a common function implementation in the base class for convenience but that's not the primary reason to use inheritance so, if it's your only reason, you should be using composition.

Answer 3

No, neither should inherit from the other. They don't pass the 'is a' test. persist_to_file is not a specialisation of calculate_tax or vice versa (note spelling).

Yes, refactor and yes, prefer composition but there are many possible avenues, beyond the scope of a plain answer.