Why Inheritance Makes Me Cry

People receiving code reviews from me on a regular basis will know this by now, but if I see inheritance happening in new code, I get moody. I look the extends in the eye and prepare for battle. I pick up my SOLID axe and start examining the principle that will allow me to slay this sub-class beast. And then I cry a little because I'll have to explain again where this centuries-old grudge comes from.

Now, don't get me wrong. I do understand the usefulness of inheritance in very specific cases. But most of the time when I encounter a subclassed beast during a code review I go all out offensive on it. Let's explore why:

Single Responsibility Principle

If you've been Object Oriented Programming for some time and you've been looking around a bit, you've definitely encountered the SOLID principles. These are basically principles (not rules) that, when followed, will help you produce code that's easier to comprehend, change and test than when you were not following them. The first principle is the Single Responsibility Principle, which states that a class should have only one responsibility, or "one reason for change".

Let's try and use that to mitigate this case of inheritance:

interface Users
{
    public function getById(UserId $id): User;
}

class UsersDB implements Users
{
    protected $db;

    public function __construct(DB $db)
    {
        $this->db = $db;
    }

    public function getById(UserId $id): User
    {
        // ...
    }
}

class UsersCached extends UsersDB
{
    protected $cache;

    public function __construct(Cache $cache, DB $db)
    {
        parent::__construct($db);

        $this->cache = $cache;
    }

    public function getById(UserId $id): User
    {
        // get from cache?

        // if not in cache
        $user = parent::getById($id);

        // put in cache

        return $user;
    }
}

Apart from all other issues that we have here, let's try to find out if the Single Responsibility Principle holds for this implementation. Looking at the class UsersDB, we can clearly see that it has just one responsibility: it represents a bunch of Users that are persisted to our database. It has no other reasons to change than for the purpose of representing users persisted in our database.

Now let's look at the UsersCached implementation. It represents a bunch of Users as well, in this case it caches them. We could think that this class has only one responsibility, until we see the parent::getById() call. It basically calls a method that we inherited (so it's available in our class) from the parent class. The knowledge needed to query the database is actually in our class because of the inheritance!

You could argue that it's still ok since it's clearly a parent class and nothing's wrong there, but let's look at it through the lense of a class should have only one reason for change. What if we decide that the constructor of the UsersDB class will now take a PDO instance instead of our own DB class? Guess what! The implementation of our UsersCached needs to change as well, because it needs to pass the PDO instance on to its parent. So a decision about the database made us change the caching class. And it's obvious that changing the caching mechanism would also affect the caching class. Which gives us more than one reason for change of this class.

This is clearly a violation of the Single Responsibility Principle.

We could fix this e.g. by using the Decorator Pattern:

interface Users
{
    public function getById(UserId $id): User;
}

final class UsersDB implements Users
{
    private $db;

    public function __construct(DB $db)
    {
        $this->db = $db;
    }

    public function getById(UserId $id): User
    {
        // ...
    }
}

final class UsersCached implements Users
{
    private $users;
    private $cache;

    public function __construct(Users $users, Cache $cache)
    {
        $this->users = $users;
        $this->cache = $cache;
    }

    public function getById(UserId $id): User
    {
        // get from cache?

        // if not in cache, get from nested user object
        $user = $this->users->getById($id);

        // put in cache

        return $user;
    }
}

As you can see, it's only a very small change.

  • we added final keywords to our classes and private to our variables to indicate that we don't want inheritance to happen 😏
  • we made both classes implement the Users interface
  • we injected an implementation of the Users interface into the constructor of the UsersCached, which will be called when no user is found in the cache.

You could try and argue that this is no better because we still rely on the Users interface to stay stable. But if that interface changes, the responsibility "representing a bunch of users" changes, which is exactly the one responsibility that we have for our class. Changing the implementation of our UsersDB class will not affect the UsersCached class anymore, except when the contract of the interface is broken. Great!

Dependency Inversion Principle

Let's take a short second look at the same code examples from above, now while trying to look at it from another one of the SOLID principles, the Dependency Inversion Principle. The principle comes down to depending on abstractions instead of concretions. In the case of the first example, the UsersCached extends UsersDB, which it needs when there's nothing found in the cache. We depend on a concrete class, which is a violation of the principle. In the second example, we made UsersCached implement the interface directly instead of extending from UsersDB. We then inject another implementation of Users in the constructor of UsersCached. Since we depend on the interface here, we don't depend on a concrete class anymore, and any implementation of the Users interface should work! This solution follows the principle! An added benifit of this is that our UsersCached instance is now much easier to test: we just need to inject a fake Users interface and see if it gets called when the cache doesn't have the value that we look for.

You could do the same with the other SOLID principles really, take e.g. Liskov Substitution Principle. If a class extends from our class, it can easily overwrite a method to behave completely differently than what it was, so the principle doesn't hold. Again, it's easy to make a violation there.

Shared Dependencies

Now that we have a few SOLID reasons to say that inheritance is not a desirable solution for our problems, let's look at some other ways that I think it is misused. Consider this example:

abstract class Notification
{
    protected $id;

    public function __construct(Id $id)
    {
        $this->id = $id;
    }

    public function toUrl($relative = true): string
    {
        $path = "/notifications/{$this->id}";

        if ($relative === true) {
            return $path;
        } else {
            return "https://domain.of.application{$path}";
        }
    }
}

I see this kind abstract classes quite often. It's a form of deduplication (people think they shouldn't repeat code), but what's problematic is the fact that you won't always know what kinds of exceptions to the rule there are going to be. Consider a class AccountSpecificNotification which extends from Notification. It wants to overwrite the toUrl() method, because account specific notifications are at /account/{accountId}/notifications/{notificationId}. Trouble is, we have to again implement the absolute and relative paths rule, and the chance of making mistakes becomes higher. Also the absolute URL of our domain is now in two classes. The knowledge about converting a notification to a URL is also in two classes. If we have a few of these exceptions, stuff gets difficult to keep track of.

As a solution, you could just make this class an interface and specify the toUrl() method on Notification instances. This way you always know the implementation for a certain type of Notification will be in the class itself. Then you'll have to live with the duplication. This is personally how I often do it. I duplicate stuff when in doubt and refactor later, with the benefit of hindsight.

Another thing that you could do is look at the toUrl() method, and see that it's in fact a shared dependency between Notifications with another responsibility than "representing a notification". It could become another class. Let's write a separate class for it:

final class NotificationToUrl
{
    public function getRelativeUrlForNotification(Notification $notification): string
    {
        $url = '/notifications/' . $notification->getId();

        if ($notification instanceof AccountSpecificNotification) {
            $url = '/account/' . $notification->getAccountId();
            $url .= '/notifications/' . $notification->getId();
        }

        return $url;
    }

    public function getFullUrlForNotification(Notification $notification): string
    {
        $path = $this->getRelativeUrlForNotification($notification);

        return "https://domain.of.application{$path}";
    }
}

Of course this is not perfect either, but it's at least a bit better from some standpoints: we get all the knowledge of converting notifications to URLs in one place, and we don't need inheritance!

In short

If you're using inheritance to fix a problem, I propose that you take a second and ask yourself why. In this post I propose a few ways of looking at inheritance that could help you come to a better solution in some cases.

  • Are you violating one of the SOLID principles? Try and look for a solution where you can adhere to them more.
  • Are you trying to remove duplication? Try to postpone your decisions about the abstraction that you're going to use to remove the duplication.
  • Do you have a shared method that multiple classes need? Try extracting it to a separate class.

Since most of the time one of these methods produces a result that I find more desirable as explained in this post, I'll mostly oppose strongly against the use of inheritance when it's not strictly needed.

Hope that you'll be with me on slaying this beast! Until next time, happy programming! 🐲

Categories: PHP