Inheritance is Bad: Code Reuse Part II
0. Overview
- Inheritance is Bad: Code Reuse Part I
- Inheritance is Bad: Code Reuse Part II
- 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.
_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.
But you don't change a Role. In this case you need to delete Destroyable first and then apply DestroyableResistances.
You also could now say you start with a class without Destroyable or DestroyableResistances and at startup you create just two anonymous classes with the one or the other role applied.
But this will really get complicated if you have a lot of different roles that you want to change.
The point is. There is a really easy solution to solve that problem. It is so easy, you don't even need Moose, a meta object, or some other fancy library to solve that problem.
First, you should generally be consuming all of your roles in a single with statement:
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.
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?
It should be more a minimal example that. But the primary reason why i didn't name it "Moveable" was more that i don't see changing a Position as movement. That's also why i named it "PositionRW". I named the method "move_by". But in real-life you would name that "add" or better overload the "+" operator. And the interface of a Position is a lot more complex, you probably also want to overload subtraction, or multiplication. And also attributes like "magnitude" and "normalized". And other methods. For example. This is the Documentation of a Vector2 in Unity: http://docs.unity3d.com/ScriptReference/Vector2.html
The Position role here is merely a minimal example that instead of using an overloaded "+" operator or an add method that adds another Vector to it, just have a "move_by" method.
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.
That is the whole purpose of this series to describe that. But you usaly use interfaces + classes (composition) for it. Not interfaces + Roles.
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.
An interface should not have any implementation at all. Because than it is not an interface anymore. There are some options to solve it. For example:
1. Create a Base "Position" Class that you inherit from
but if you do that, you end up with inheritance once again and i'm talking about problems with LSP if you inherit from such a base class and make fundamental changes how the class work.
2. Create another Role
That was one way how you mentioned it to solvwe it, well you could do that. But creating a Role for helpers that will only inject to another Role sounds really strange. Such a design would probably work, but i see a lot of other problems with such a design.
3. Create an interface
That is actually what you did in your example. But i don't see any improvement at all in your code. Actually it even gets more worse and you end up exactly with the "code duplication" that i mentioned. You have created an Interface::Position. and now you use that Interface in Position and Position::Movable
And if you look exactly, you have just Copy & Pasted code. Actually you have Position and you implemented the "x" and "y" Attributes in it. And in Position::Movable you re-implemented those attributes once again.
Yes, you have written a seperate interface, but you don't have any code-reuse at all in your example. You just re-implement the exact same interface. Sure with just two fields "x" and "y" that doesn't sound much. But this is exactly why i mentioned what would happen if your Position gets more complicated. Imagine it is a Vector2 exactly like i mentioned with functionality like in Unity. You absolutly don't want to re-implement all methods, static functions or attributes once again in Position::Movable. But this is exactly what you do.
4. Create a seperate class and use Composition
The last step what you can do is to write a Position Class instead of a Position Role. And that is exactly what i do in Part III.
And this aproach has no code duplication at all. And you only need to write a single immutable version. Something like PositionRW or Position::Movable is absolutly not needed at all. Your Unit or Building just have in instance of this Position class, and in Part III i mentioned it also "Vector2" instead of "Position". Your Building with a fixed Position have this.
has 'position' => (
is => 'ro',
isa => 'Vector2',
required => 1,
);
and a Unit that can change the Position looks like this.
has 'position' => (
is => 'rw',
isa => 'Vector2',
required => 1,
);
While you needed to write two roles "Position" and "Position::Movable" with Code-Duplication between those classes, the only thing i needed was to change the is => 'ro' to is => 'rw' to have a Position that can change, and another that is always fixed.
So if you now say:
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.
The problem is. You believe you don't have any code-reuse. Exactly like you believed that your code version has no code-reuse. But i already pointed that your Position::Movable is overall just a big code duplication in your design. The problem is that if you want similar things you end up quickly creating new roles, even if the new thing what you need does exactly what a role before does.
Just a simple example. A Unit has a current Position. Imagine a top-down 2D game where you click with your mouse where your unit should walk. And usually your character then starts to move to this position (and sure it needs some time to do that). What you then need is two Position. A Current Unit Position and a Target Position. In my Design i would just two positions.
has 'current_position' => (
is => 'ro',
isa => 'Vector2',
required => 1,
);
has 'target_position' => (
is => 'ro',
isa => 'Vector2',
required => 1,
);
what would you probably end with? Well because your "Position" and "Position::Movable" both uses "x" and "y" cou cannot inject both into your class. And you also cannot inject exactly the same role two times into a single class that works independently.What you would end up is probably a new Role, once again. Probably a "Positon::Target" Role? That once again reimplent "x" and "y" but are named "target_x" and "target_y", or something like that?
And that is more exactly what i mentioned that with Roles you don't have much Code-Reuse and a lot of Code Duplication. Because the only thing you really needed to change in a Positon::Target role to a Position Role was just the names of the attributes, even if they do exactly the same.
If you work with Roles you have such a Code-Duplication a lot. And the thing is. You will probably even not notice such a Code Duplication. Because a Position::Target has other attribute names as Position or Position::Movable. So it looks like you have to do that.
And that is exactly what i try to explain in Part III of the series. That you even get better Code Reuse and eliminate such Code Duplication with Composition.
If your building has also a shild not just life. Well you just use two Destroyable objects to represent the state of the shild and the life. While a design with roles you probably would end up with a "Shild" Role. That does exactly the same things like Destroyable, just with some other attribute names, because you can't reuse the Destroyable role twice.
And you probably still would think that you don't have any Code-Duplication at all.
You also mention breaking the Liskov Substitution Principle. I don’t see how that applies if you eliminate inheritance and only use roles.
I mentioned breaking LSP only if you would do Solution Nr. 1 from above. That means, creating a base class that you inherit from to solve the "two roles with some shared code" problem. Actually your code that you provided is just a "Copy & Paste" Solution with no Code-Reuse at all.
Interesting is more that you still think you don't have any Code-Duplication. That is also the reason why i consider Roles harmful. Because they don't really solve any problem at all.
And yeah, i also have already designed big systems with roles. And when i switched to a Composition solution and created classes instead of roles i feel rather dump to ever used Roles and didn't saw that Code-Duplication all over the place.