2

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?

Improve this question
7
  • 4
    Does this answer your question? Why should I prefer composition over inheritance? Commented Feb 3, 2022 at 14:25
  • 4
    Class MyDoEverythingClass -- Trouble already. See "Single Responsibility Principle." Commented Feb 3, 2022 at 16:39
  • Code is illustrative :D Commented Feb 3, 2022 at 18:02
  • 1
    I agree with @RobertHarvey - the actual refactor you need is to decide which methods belong in the same class and which don't. Data validation and persistence have nothing to do with plotting, which has nothing to do with tax computation. I know that these are illustrative methods, but the code won't magically improve just because you decided to use inheritance or not. Classes should represent things that make actual sense because it helps us understand our own code. If you're just using them as a repository for loosely related methods, just use a regular module with free functions instead. Commented Feb 3, 2022 at 20:10
  • 1
    @MYK "it's hard to maintain cohesion without tight coupling" Beware the Dunning-Kruger effect. "Tight" coupling is a relative label that only applies in cases where a looser coupling is available. You can't judge the tightness of a coupling when you don't see a looser way to couple your components. Commented Feb 4, 2022 at 12:46

3 Answers 3

Reset to default
4

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()
Improve this answer
3
  • 3
    Seeing SomeHelpers() hardcoded here made me sad. Is there a reason not to pass it in when constructing PublicFunctionality? Commented Feb 3, 2022 at 18:57
  • 4
    @candied_orange Not everything needs dependency injection. If this just provides internal helper functionality, exposing it as part of the public interface of that class would be fragile and counter-productive. Of course, if the helper class were more like a default strategy, then making it possible to override it could make sense. Hard-coding the SomeHelpers class is no worse than hard-coding the step1 and step2 functions in my second example. But if the helpers were to have side effects and if this was larger-scale software, I'd agree with you. Commented Feb 3, 2022 at 22:49
  • I agree that the real needs are important. Just not sure what good going to all the trouble of making it a separate class is doing you. Commented Feb 4, 2022 at 1:30
1

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.

Improve this answer
1

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.

Improve this answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.