Inheritance is Bad: Code Reuse Part II

0. Overview

  1. Inheritance is Bad: Code Reuse Part I
  2. Inheritance is Bad: Code Reuse Part II
  3. Inheritance is Bad: Code Reuse Part III

1. Introduction

My first part about Inheritance was more theory. I explained the problems with Inheritance, and explained how roles can solve some of them. I told that there is even a better way, but instead of going straight to that point i want to first talk about Roles. We first need to understand Roles and which problems Roles still have to go further. Instead of just explaining the problems in theory i also want to add some code, because code always helps a little bit more to understand the problem. So that post even got a little bit longer than i really wanted.

2. Our example

To point out the problems with Roles we should create a little example.

In this example i want to create two classes of a game. In that game i should be able to define Buildings and Units. Every Building in the game has a name, can be destroyed and have a position. The exact same goes for a unit. The only difference is that the position of a building is not changeable, but a unit can easily move and change his position.

Lets just quickly talk about inheritance. I hope from the first post that using inheritance is not a good way. Yes you can solve the problem if you start inherit from one or the other things. But you later will run in the problems that described in Part I. If you read Part I and still don't see why inheritance would be a bad way to solve please just comment.

3. Behaviours

At first lets think which behaviours we need to implement as Roles. The best way how to do it is to just try to implement a class. At best is to start with the smallest classes. The reason is that you you have implement them fast and you already get some Roles that you can reuse in some bigger classes.

Well here we just have Building and Unit. But Building has an immutable Position so we start with the Building class. Well the Building class was already defined well. We have a name, it is Destroyable and it has a position. So we can think of 3 Behaviours/Roles "Name", "Destroyable" and "Position".

4. "Name" Role?

Lets think of the name. Well it really depends if you really create a behaviour out of it.

In our game we just set or get the name. It is just a single string propertie and thats it. So even if nearly every class have a name, it doesn't make too much sense to create a role out of it. But if in your application you have a lot of methods associated with "name" then you should definitly create a role. As soon as you hit 2 properties it usaly makes sense to create a role.

Even with one propertie you can argue that this should be role. Because adding a role is still less code. And if you already can think of additions to the "name" propertie that every class with a "name" should have you can directly start with a role. So creating a role even for a single propertie is not wrong. But in that case i really can think of anything special or what i want to add. I will just set/get the name and that's it. So i implement "name" just as a simple propertie in every class.

5. Destroyable Role?

Now "Destroying" starts to get a little bit more complex. In order to destroy something we need a way to express "life". What i use is just an integer for that. When our integer gets to 0 our Building is destroyed. But we also want a start value or better a maximum value. That is important because that means we need to ensure that the value not be less 0 or more than our maximum value.

So just having a "life" propertie that's just an integer and can directly be changed is not a good way. Instead we should create a role with a current value propertie. A proertie that defines the maximum, and some helper properties. For example a boolean that just return true/false if our Building is still alive or not.

We also should define "ro" properties not "rw" properties. Actually defining "ro" properties doesn't mean we create immutable classes/roles. In my case what i want to exporess is that a user is not allowed to change that value directly. But you still can provide methods that change the value. For example currentlife. If you allow a user to directly change currentlife he could set the value lets say to 120 or -10 even if 100 was the maximum allowed value, and the value should never go under 0.

Instead we enforce that the user have to call addlife() or subtractlife() if he wants to change the life. And those methods ensure the correctness of the current_life propertie.

So we create a behavior named "Destroyable". This behaviour will look something like this.

Role: Destroyable
  - int max_life
  - int current_life
  - bool is_alive
  - void add_life(int x)
  - void subtract_life(int x)

In code that Role above can look something like this:

package Destroyable;
use Moose::Role;

has 'max_life' => (
    is  => 'ro',
    isa => 'Int'
);

has 'current_life' => (
    is     => 'ro',
    isa    => 'Int',
    writer => '_set_current_life',
);

has 'is_alive' => (
    is      => 'ro',
    isa     => 'Bool',
    default => 1,
    writer  => '_set_is_alive',
);

sub add_life {
    my ( $self, $amount ) = @_;
    my $max = $self->max_life;
    my $cur = $self->current_life;
    if ( $cur + $amount <= $max ) {
        $self->_set_current_life($cur + $amount);
    }
    else {
        $self->_set_current_life($max);
    }
    return;
}

sub subtract_life {
    my ( $self, $amount ) = @_;
    my $min = 0;
    my $cur = $self->current_life;
    if ( $cur - $amount >= $min ) {
        $self->_set_current_life($cur - $amount);
    }
    else {
        $self->_set_current_life(0);
        $self->_set_is_alive(0);
    }
    return;
}

Now we have a "Destroyable" Role that can be reused in a lot of classes.

6. Position Role

The last thing we need for our Building is a "Position" Role or behaviour. Let's assume we have a 2D game and the position are just integer numbers. The only thing we need are X and Y coordinates. Does it make sense to create a role out of it for just 2 properties?

Well yes, even if the coordinate is just expressed with a single propertie (for example a 2D Vector) it makes sense to do it. The difference between the name where we didn't do that is if we can think of the position as more as just a single value.

And we already thought about a unit that has a position that can be moved. So it could make sense in the future to add some methods to it. But we could also add a lot of other helper methods to it or change the way how position works. For example we really write a Vector2 class, but still has an x and y value, or methods that transform our positions to other types and so on.

So now we have a simple Position Role like this.

package Position;
use Moose::Role;

has 'x' => (
    is  => 'rw',
    isa => 'Int',
);

has 'y' => (
    is  => 'rw',
    isa => 'Int',
);

7. Building class

Now we have everything for our Building class that is really small and look like this.

package Building;
use Moose;

with 'Destroyable';
with 'Position';

has 'name' => (
    is  => 'rw',
    isa => 'Str',
);

sub info {
    my ( $obj ) = @_;

    my $alive = $obj->is_alive ? "true" : "false";
    return sprintf("[Building: Name=%s Position=%d/%d Life=%d/%d Alive=%s]\n", $obj->name, $obj->x, $obj->y, $obj->current_life, $obj->max_life, $alive);
}

I also added a info() method to it that represents the current state of out object in a string. Now we can write code like this.

my $hq = Building->new(
    name         => 'Headquarter',
    max_life     => 100,
    current_life => 100,
    x            => 10,
    y            => 10,
);

print $hq->info;
$hq->subtract_life(70);
print $hq->info;
$hq->subtract_life(70);
print $hq->info;

8. Unit class

The reason why we created Behaviours/Roles was to ensure we could reuse our code. Now let us think of our Unity class. The unity class can do exactly what the Building class can do, the only thing that change is that the position can change. So what we now can do is create a "Unit" class and add the "Destroyable" role to it again. But what is with Position?

Now we face one Problem. We have written code for Position, even if it is not big at the moment it correctly represents a Position. But at the moment the x/y values are read-only. That is also correct because we wanted a Position that can not be changed. Our building should not be able to move.

If we start to change the Position role and we change x/y to "rw" we have the problem that also Buildings can change there position and we don't want that to happen.

We could also let x and y be "ro" and add some methods that changes the Position. And than add a propertie where we can configure if position can change or not. But in my opinion. That solution is even more worse, it is exactly one of the same problems we got with inheritance. The thing is. If we add a role like "Position" and it has some methods in it, we should be able to call it, and write code that disables the methods. Its just an evil workaround to do that. Than you also can switch back to inheritance and or your booleans there to activate/deactivate methods, you literally have nothing won if you start using workarounds like that. Because the whole point of roles was that you only add to a class what matters, and you can freely reuse pieces without restrictions like an inheritance tree.

If we really think of it, the only way how it seems we could do it "right" is to duplicate some code. So we will write another Position Role. But this time x/y can be changed. We still make x/y "ro" attributes, but we provide some methods to change it. Lets name the class PositionRW

package PositionRW;
use Moose::Role;

has 'x' => (
    is     => 'ro',
    isa    => 'Int',
    writer => '_set_x',
);

has 'y' => (
    is     => 'ro',
    isa    => 'Int',
    writer => '_set_y',
);

sub move_by {
    my ( $self, $x, $y ) = @_;
    $self->_set_x($self->x + $x);
    $self->_set_y($self->y + $y);
}

And now our Unit class looks something like this

package Unit;
use Moose;

with 'Destroyable';
with 'PositionRW';

has 'name' => (
    is  => 'rw',
    isa => 'Str',
);

sub info {
    my ( $obj ) = @_;

    my $alive = $obj->is_alive ? "true" : "false";
    return sprintf("[Unit: Name=%s Position=%d/%d Life=%d/%d Alive=%s]\n", $obj->name, $obj->x, $obj->y, $obj->current_life, $obj->max_life, $alive);
}

If we write code like this:

my $hq = Building->new(
    name         => 'Headquarter',
    max_life     => 100,
    current_life => 100,
    x            => 10,
    y            => 10,
);

print $hq->info;
$hq->subtract_life(70);
print $hq->info;
$hq->subtract_life(70);
print $hq->info;


my $unit = Unit->new(
    name         => "Hero",
    max_life     => 100,
    current_life => 100,
    x            => 20,
    y            => 20,
);
$unit->add_life(20);
$unit->subtract_life(50);
$unit->move_by(5, 10);
print $unit->info;

We get this output

[Building: Name=Headquarter Position=10/10 Life=100/100 Alive=true]
[Building: Name=Headquarter Position=10/10 Life=30/100 Alive=true]
[Building: Name=Headquarter Position=10/10 Life=0/100 Alive=false]
[Unit: Name=Hero Position=25/30 Life=50/100 Alive=true]

So everything works. We now have our Behaviours. The behaviours are reusable. We have a Position class that is immutable and an immutable Position class. Our Unit class is a really good example how we reused only that code what we needed. So is everything good and are we in heaven?

9. Problems with Roles

Now lets rethink, do we have any problems in our code so far? At first lets talk about what a "problem" is. Sure our code works exactly like we wanted. We have a good Code Reuse. But in my opinion our Code Reuse is not good enough.

And you see that Problem with the PositionRW Role. Well the problem is not that we have a PositionRW Role. To have two Roles like that is absolutly fine, as long as there implementation differs. In that case we have a Position that is immutable. And PositionRW is also a position, but it is a mutable Position. Both are Positions, but they have a different implementation.

So having two roles for two different positions is okay. But what is annoying is that we literaly duplicate code. Well in that example its just 2 properties x and y, but it is still annoying that we have to do that. It is not important here that there are just 4 lines of code. Lets think further. What would be if we also implemented 5 helper methods? Well we also would copy all Methods from Position to PositionRW because PositionRW has the exact same functionality. Or what is if we want to add a method or later change a method because some had a bug? Well you will run in the problems that you usally get through duplicated code.

If you now think that inheritance could probably help you. Not really. I don't knew exactly at the moment if Moose lets you redefine a propertie from 'ro' to 'rw' in a subclass. But even if it allows that, i would never recommend that. You will break the rules in the Liskov Substituation Principle (that will be another blog post) that even will create a greater mess.

And often someone uses 'ro' because of thw reasons i explained before. To ensure that the object is always in a correct state. Changing a property from 'ro' to 'rw' means that methods could be wrong, for example because you created lazy methods that just calculate the value once, because well the value never changes or when it changes through an allowed method then you reset your cache. There are ways how you program everything that it still can work even with a 'rw' propertie. But if someone did that he would not have set the propertie set to 'ro'. Using 'ro' properties usally means you make your life easier, because you don't allow changing or just through your supported methods that can handle with that.

If you start with 'rw' and inherit and change it to 'ro' then you have the same problem like already mentioned. Code that was to suppose to change a value will now throw errors if you execute them. That is just a crazy design if you work in such a system. Because you never knew if when a error is thrown because something is wrong, or something got disabled. And actually makes it even right that there is thrown an error. You should make everything simple. If you are not allowed to change something it should be 'ro' and you should not have a method that can change it, but throws exception when you try executing them. It's just an insane design.

But how can we describe our problem in words? Well i name it "Multiple implementations". So what does "Multiple Implemenation" means? In our case we have two different Positions. One should be immutable, the other mutable. In such cases you often have some code that overlaps. But even if there is something that overlaps it would be great if we could still avoid Code Duplication as much as possible.

The problem here is. Role itself doesn't helps use with Multiple Implementations at all. In fact Roles makes it even more worse. Because your code is locked in a Role that can't be used directly. You only can apply a role to a class, and then use that class.

To better describe Multiple Implementation lets look at another example. Lets relook at our Destroyable Role. The role itself works like wanted. But lets assume we wanted to add a Dragon. Now our Dragon does fire damage. But wouldn't it be stupid if a dragon could be beaten by fire? Well yes, i think so, so i want to add that dragon in my game and make it immun to fire. But how do i do it?

So everything that can be damaged gets the Destroyable Role. To allow our dragon to be immung against fire we need to change the way how Destroyable works. We could for example invent a DestroyableResistances Role. In that role we can for example fire resistances, and our Dragon then becomes 100% fire resistances. The problem if we do it, you will see that a lot of your code will overlap. You probably just start with copy Destroyable.pm to DestroyableResistances.pm and start to change your code.

I hope that it is now understandable what i mean with "Multiple implementations", and that Roles still can't help you in such a case.

And now you say to me. Okay, but i can change Destroyable and add Resistances to it, right? Well sure, you can do it. And if you want to add another feature you also can add it to Destroyable. And another feature, and another feature, and... and later you end up again with a class 1000 lines long that has nearly every feature build in. And how do you configure which feature is active or deactivated? Well probably again with some boolean values?

You do not learn, or do you?

But here comes another problem. Lets assume you decided to create different Roles. Now you have your Destroyable Role for Buildings. Your Dragon gets your shiny new DestroyableResistances Role. You now think. "Yes we have Code Duplication, but i accept it". It isn't much and the design is much cleaner then having one big class with every feature in it. Okay that is fine, but suddenly your Game Designer comes to you. He wants you to add something new. A Hero class. So what is special about your Hero class? The Hero class starts with no special. But when it hits level 2 the player can choose if he wants two different armors. The first Armor gives the user 50% fire Resistances, and the second armor heals every friendly unit around the hero when our hero gets damage.

And now you are stuck. You Hero starts with your Destroyable Role. Well you already have a DestroyableResistances role, but you can't change a role at runtime. And even special, not just change the role in the class, you need to change the behaviour from Destroyable to DestroyableResitances at runtime just for a specific object, not for the whole class. Okay, maybe you just start with DestroyableResistances and set resistances to 0% to emulate a normal Destroyable role. But how do you implement the healing? That's a new feature. Creating a DestroyableHealing Role is possible but you can't change from DestroyableResistances to DestroyableHealing.

Will you probably once again throw the different roles away and just stick to your DestroyableThatDoesEverything role? That good old role with 2000 lines of code where all different 16 hit mechanics that evolved over time is now included?

The next problem that roles don't solveare that shiny big objects. Objects where you look at and getting 200 properties/methods. Often you get it from objects that use inheritance. It just inherit from one class, that inherit from another class and so on. And later you have your high level class that has so much methods and properties that it takes forever to understand what that class can do.

And if that is not enough, there is still another problem. All your properties and methods you define can be considered as "global". If you want to add a role to your class, then the role can conflict with another role. For example you have an "Add" Method in Role1 and an "Add" method in Role2. Then you get problems. You can't just add both functions. And that can create serious problems. If you have that in mind it means that you have to pick the names of all your properties and methods wisely as if they were global. Because if you don't do that, you can't reuse your code. And once again its the same problem with inheritance. If you inherit from another class you also need to look that you don't overwrite a method accidental. You have to name every new method unique.

10. Next Part

To finish this part, in my opinion roles are by far better than inheritance. It is just a cleaner design and you achieve code re-usability more easily. But there are still some problems with Roles. If you need Multiple Implementations then roles don't help you at all with code re-usability. Either way you still start to copy & paste your code into new similar roles. Or you start writing your big roles that does everything with a lot of booleans to decide what to do. And that was exactly where you end usually with single-inheritance and was one reason for roles. To pick everything into behaviour and just add to a class what it really needs, instead of depending on a lot of state variables, that makes everything more complex.

So in the next part i will describe a way how you can also eliminate the remaining problems. How you can write multiple implementations. Switch those easily at runtime. How you don't run in naming problems, and how to design objects that are easy to understand without hundreds of properties/methods and you achieve an even better code re-usability.

4 Comments

_you need to change the behaviour from Destroyable to DestroyableResitances at runtime just for a specific object_

Roles can be applied to an instance. See "Applying Roles" in Moose::Role.

First, you should generally be consuming all of your roles in a single with statement:

with qw(Destroyable PositionRW);

By composing those separately as you have (two separate with statement),you’re lose the composition safety of roles and your code is reduced to little more than mixins.

Your concerns about separate Position roles don’t make sense to me. One is mutable and the other is not. Since position and movement are different things, why not make them different roles in your example? And you can define a position interface role to guarantee that even though one position is mutable, the exposed contract is immutable. This is why many languages separate interface from implementation: sometimes you want the same interface, but a different implementation. I’ll show my full code below.

You wrote regarding the two position roles: Lets think further. What would be if we also implemented 5 helper methods? Well we also would copy all Methods from Position to PositionRW because PositionRW has the exact same functionality.

No, you wouldn’t do that. If you have shared behavior, you don’t cut and paste the code. You would put it in another role or, in this case, perhaps in the interface role that you’re sharing.You mention code duplication repeatedly in your post, but I have built a number of large-scale systems with roles and I never have this issue.

You also mention breaking the Liskov Substitution Principle. I don’t see how that applies if you eliminate inheritance and only use roles.Subclasses don’t exist and aren’t more specialized. Different classes are merely different.

What follows is a quick ’n dirty (and incomplete) cleanup of your code.

use 5.14.0;
use warnings;

package Interface::Position {
use Moose::Role;
requires qw(get_x get_y);

sub position_info {
my $self = shift;
return sprintf "X=%d Y=%d" => $self->get_x, $self->get_y;
}
}

package Position {
use Moose::Role;
with qw(Interface::Position);

# use forward declarations to work around
# https://rt.cpan.org/Public/Bug/Display.html?id=46347
sub get_x;
sub get_y;
has 'x' => (
is => 'ro',
isa => 'Int',
reader => 'get_x',
);
has 'y' => (
is => 'ro',
isa => 'Int',
reader => 'get_y',
);
}

package Position::Movable {
use Moose::Role;
with qw(Interface::Position);

sub get_x;
sub get_y;
has 'x' => (
is => 'ro',
isa => 'Int',
reader => 'get_x',
writer => '_set_x',
);
has 'y' => (
is => 'ro',
isa => 'Int',
reader => 'get_y',
writer => '_set_y',
);

sub move_by {
my ( $self, $x, $y ) = @_;
$self->_set_x( $self->get_x + $x );
$self->_set_y( $self->get_y + $y );
}
}

package Destroyable {
use Moose::Role;

has 'max_life' => (
is => 'ro',
isa => 'Int'
);

has 'current_life' => (
is => 'ro',
isa => 'Int',
writer => '_set_current_life',
);

has 'is_alive' => (
is => 'ro',
isa => 'Bool',
default => 1,
writer => '_set_is_alive',
);

sub add_life {
my ( $self, $amount ) = @_;
my $max = $self->max_life;
my $cur = $self->current_life;
if ( $cur + $amount $self->_set_current_life( $cur + $amount );
}
else {
$self->_set_current_life($max);
}
return;
}

sub subtract_life {
my ( $self, $amount ) = @_;
my $min = 0;
my $cur = $self->current_life;
if ( $cur - $amount >= $min ) {
$self->_set_current_life( $cur - $amount );
}
else {
$self->_set_current_life(0);
$self->_set_is_alive(0);
}
return;
}
}

package Nameable {
use Moose::Role;
has 'name' => (
is => 'rw',
isa => 'Str',
);
}

package Unit {
use Moose;

with qw(Position::Movable Destroyable Nameable);

sub info {
my ($self) = @_;

my $alive = $self->is_alive ? "true" : "false";
return sprintf(
"[Unit: Name=%s %s Life=%d/%d Alive=%s]",
$self->name, $self->position_info, $self->current_life,
$self->max_life,
$alive
);
}
}

package Building {
use Moose;

with qw(Position Destroyable Nameable);

sub info {
my ($self) = @_;

my $alive = $self->is_alive ? "true" : "false";
return sprintf(
"[Building: Name=%s %s Life=%d/%d Alive=%s]",
$self->name, $self->position_info, $self->current_life,
$self->max_life, $alive
);
}
}

my $hq = Building->new(
name => 'Headquarter',
max_life => 100,
current_life => 100,
x => 10,
y => 10,
);

say $hq->info;
$hq->subtract_life(70);
say $hq->info;
$hq->subtract_life(70);
say $hq->info;

my $unit = Unit->new(
name => "Hero",
max_life => 100,
current_life => 100,
x => 20,
y => 20,
);
$unit->add_life(20);
$unit->subtract_life(50);
$unit->move_by( 5, 10 );
say $unit->info;

Leave a comment

About Sid Burn

user-pic I blog about Perl.