A tale of growing up, and realizing your heroes have flaws
The Beginning
Almost nine years ago, when I was near the end of my BSc, I was sure that inheritance is the greatest thing an OO programmer has in their arsenal of tools. During most of my studies, we were taught the principles of OOP, why interfaces are important, and how inheritance is a great tool for reusing code, abstracting your code and design, and what a generally powerful tool it is. That's not incorrect. It is good for those things. But it took me a few years to realize that when I idolized those concepts, I tended to use inheritance where it shouldn't have been used.
We were shown the example where inheritance fails:
A circle, and an ellipse. Which is the abstraction of which?
But we were also told examples like this one:
class employee {
pay()
generateHoursReport()
assignVehicle()
}
class Manager extends Employee {
assignEmployee()
}
class CEO extends Manager {
}
When you think about it in terms of technical abstraction, it makes perfect sense. A CEO is a manager. A Manager is an employee. A CEO is an employee.
All three share the basic functionality of the employee, and each extends those functions, and/or add new functionality. This solution is exactly the solution I would come up with, before I started to internalize the first of the SOLID principles, and learned more about the delegation/composition pattern.
Single Responsibility Principle
Robert C. Martin wrote about the SOLID principles in the Agile Software Development and the definition was that:
A module (class, interface, package,…) should do only one thing.
It was later refined by Martin to state
A module should have only one reason to change
I won't go into details on the meaning of the refinement, and you'll find a lot written about it already, especially in Martin's own blog. But let's apply this principle to our Employee example.
Looking at the Employee class, we can see that it handles three things: financials, motor department, and HR subjects. To discuss it with Martin’s refinement in mind, there are three departments that might give a reason to change something in the employee class.
- Payroll department decides to update the way they calculate the salary of employees.
- Motor department wants to make it so only managers can be assigned vehicles due to budget cuts.
- HR replaced their reporting system and require a new template for the hourly report.
It took me a while to understand why this is even an issue. I would think that the Employee class knows how to handle the specifics for its own level, and once you travel up the chain, the Manager will probably override the things it needs, and so on.
But...
You implement the
pay(finance) andgenerateHoursReport(HR), and quickly notice a piece of repetitive code between the two that counts the total amount of hours. You’ve read about code smells and you want to be DRY, so you refactor it out to a third, private method:getTotalHours.
Months later, HR wants to change the report so that it shows the remaining hours. The new developer is not familiar with the system and changesgetTotalHoursso that it returns the value times -1.generateHoursReportnow works as expected, butpayis now very broken. The change requested in one method affected the other.We have this structure, and now we want to introduce a new type of employee:
Contractor. The contractor needs to generate a hours report, but not to your system, and your system does not need to pay him because you pay his company. You want to have Contractor extend Employee, but the new class should not include a pay method. Maybe have Employee extend the Contractor, but an employee is not a contractor.
So you start to introduce base classes that do not actually represent anything, it’s just a “base” class to extend from.
I promise you this: a year later, your simple inheritance structure became a huge inheritance tree, with classes likeRootEmployee,ContractorWithVehicle, andTechnicalManagerthat cannot have any employees.
How can we change Employee and its structure to avoid these issues? How can we design the system to support all of these departments?
One way to do it, is to compartmentalize the logic to three different parts, and change the design to one of composition & delegation. This creates four pieces of code, each responsible for one topic: HR, Payroll, Motor, and the Employee that composites the three and interfaces with them by providing information for the relevant employee. With this modification, our design now looks like this:
interface IPaymentStrategy {
pay()
}
class SalaryEmployeePayment implements IPaymentStrategy {
pay() {...}
}
interface IReportFactory {
generateHoursReport()
}
class HourlyEmployeeReportFactory implements IReportFactory {
generateHoursReport() {...}
}
interface IVehicleDecorator {
assignVehicle()
}
class LeasingVehicleDecorator {
assignVehicle() {...}
}
class Employee {
private IPaymentStrategy paymentStrategy
private IReportFactory reportFactory
private IVehicleDecorator vehicleDecorator
public Employee(IPaymentStrategy p, IReportFactory r, IVehicleDecorator v) {
...
}
pay() { this.paymentStrategy.pay() }
generateHoursReport() { this.reportFactory.generateHoursReport() }
assignVehicle() { this.vehicleDecorator.assignVehicle() }
}
var employee = new Employee(
new SalaryEmployeePayment(),
new HourlyEmployeeReportFacotry(),
new LeasingVehicleDecorator()
)
var manager = new Employee(
new ManagerPayment(),
new HourlyEmployeeReportFactory(),
new LeasingVehicleDecorator()
)
var ceo = new Employee(
new ManagerPayment(),
new HourlyEmployeeReportFactory(),
new CompensationVehicleDecorator()
)
With this separation, we only need one Employee class, and the inheritance is provided by giving it different behaviors for each part.
Replacing the behavior of one element has no effect on the other parts of the system, which is basically the core idea behind the single responsibility principle.
Inheritance may still be used, but it will be applied to a single element of behavior, and will not cause the ContractorWithVehicle type of problem or create a need for multiple inheritance.
Composition & Delegation
Composition does not necessarily mean delegation. The composition design pattern describes a way to define an object by its connection to other objects: TreeNode, or ListNode are the most basic of examples for it.
Delegation does not necessarily mean composition. Delegation (as is implied by the name) can be used when we want our object to fulfill a specific API, without being connected to the implementation of it.
The above example, and plenty of others like it (like GameObject) show how using inheritance can sometimes lead to poor design, especially when the inheritance is already implemented and more, different usages appear.
The composition design has more benefits:
- It makes our design more testable, since each part of the code can be tested completely separately
- It makes our design more aligned with the Liskov Substitution Principle
- It makes our design more aligned with the Interface Segregation Principle
Composition and Delegation together, make us look at our classes in terms of behaviors, and not as only “X is a Y”.
Wrap up
I want to stress one final thing about “loving” a concept. In the past, thinking that a specific design is best often got me to a place where I tried to fit the problem to the design, and not the other way around.
Remember that you have more than a hammer. Don't see your problems as only nails.
Top comments (2)
Not any one pattern is good for everything, but in the case of inheritance, that particular example describes an incompetent developer rather than a negative point against inheritance: a developer should fully understand the code they are working in.
As an example, I could say that functional programming (without classes) is flawed, and I could change one word in that example, but it won't do justice to the topic of describing actual flaws:
Maybe the article can describe why exactly the developer felt inclined to modify a function/method without checking how it propagates into the rest of the program. Was there actually a reason inheritance made it more likely for the developer to make that mistake in that example?
A bad developer can break any system...
I've had the misfortune of working on systems that had a giant inheritance tree, and it can get extremely difficult to understand completely all possible connections.
I agree that the example is "too simple" and has the blame on the developer, not the design. You could also claim that a good suite of unit tests would detect this breaking change.
But, with inheritance you could get into a situation where the two functionalities can share the same code, exposing the system to this type of breaking change. Something that is much less likely to happen with a less coupled approach of composition and delegation.