Jump to content

  • Log In with Google      Sign In   
  • Create Account

single responsibility principle in practice


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
14 replies to this topic

#1 Tispe   Members   -  Reputation: 1032

Like
2Likes
Like

Posted 07 June 2013 - 05:53 AM

Hi, I would like to know how to practice the single responsibility principle.

 

My first thought is that I should seperate Data and the functions that manipulate them, so that each manipulation requires its own class. But that seems counter intuitive since in OOP, a class has all the methods to manipulate its own members.

 

Lets take an "Entity class".

members:

-Position

-Health

-Ammo

-Skeleton

-Mesh

 

methods:

-Move()

-Shoot()

-TakeDamage()

 

Now, we suddenly have a class which does alot of things, but the single responsibility principle says that the class should do only one thing.

 

How then do we practice the single responsibility principle in this example?

 

My guess is that all the methods are taken out and the members are made public. We have an array of Entities that has to go through a Move class, a Shoot class, a TakeDamage class and so on. But that seems wrong, it just makes the code more complex.

 

What is appropiate to have in the Entity class with respect to the single responsibility principle? Should helper functions or other class methods manipulate the Entity members?



Sponsor:

#2 RobTheBloke   Crossbones+   -  Reputation: 2342

Like
0Likes
Like

Posted 07 June 2013 - 06:19 AM

My first thought is that I should seperate Data and the functions that manipulate them, so that each manipulation requires its own class. But that seems counter intuitive since in OOP, a class has all the methods to manipulate its own members.

But the low level hardware dictates that you should process data in batches. Typically I value the opinion of hardware more than sticking to 'good' OOP principles.

For example, having an 'EntityMovement' class to handle the movement of all entities might be a good idea (since it gives you a nice way of testing for collisions between all entities, controlling group motion, etc).



#3 Juliean   GDNet+   -  Reputation: 2605

Like
0Likes
Like

Posted 07 June 2013 - 06:22 AM

How then do we practice the single responsibility principle in this example?

 

One option would be to use an entity/component system. Instead of having "health", "position" as member, and some methods like "shoot", etc... you'd have a generic entity class without any real data members. You'd then have a "Health-Component", and a "Position-Component", that you can attach to this entity-class. Those components would only contain data - float health, vector3 position, you get the idea. Then you'd have systems, that would iterate over all entities based on the components they have, and perform the actions - a "Move" system can manage the change in position, for example. (these are all just primitive examples - you wouldn't want to have a pure "Health" component or a "Move" system in any larger project, really).

 

Have a look at https://github.com/alecthomas/entityx, if you eigther use C++11 its a good starting point, otherwise you'd at least get an idea about the implementation. That way, you have the responsibilities optimally distributed amongs multiple modules/classes.


Edited by Juliean, 07 June 2013 - 06:23 AM.


#4 Ravyne   GDNet+   -  Reputation: 7330

Like
5Likes
Like

Posted 07 June 2013 - 02:47 PM

The Single Responsibility Principle (SRP) doesn't strictly mean that mean that no class should ever do more than one thing. A slightly more accurate definition would be that adhering to the SRP means that one class expresses one single concept, and as much as is reasonable, utilizes other classes and interfaces to express its workings, so as not to be bound to the low-level details of such external systems.

 

So, taking your example, its perfectly reasonable, under the SRP, to have an entity which holds all those things that you mentioned, as long as those which are not primitive types are reasonably encapsulated, and themselves hold to the SRP. In other words, an Entity which is made up of a Position class, a Mesh class, a Skeleton class, and primitive data members representing health and ammo, still expresses the singular concept you call "entity". When an instance of the Entity class is made to change its position, it uses the interface of its position member to affect that change in itself; the same goes for functions related to its skeleton and mesh members. Note that this is a reasonable choice, but far from the only one (in fact, I'd probably make health and ammo distinct classes, as they have behave differently than their underlying primitive types), and also that the Entity class might make itself up through a combination of composition, reference, inheritance, or component-entity systems.


Edited by Ravyne, 07 June 2013 - 02:55 PM.


#5 L. Spiro   Crossbones+   -  Reputation: 13576

Like
1Likes
Like

Posted 07 June 2013 - 04:05 PM

Tispe, on 07 Jun 2013 - 20:44, said:
Lets take an "Entity class".
members:
-Position
-Health /* Wrong */
-Ammo /* Wrong */
-Skeleton /* Wrong */
-Mesh /* Wrong */

methods:
-Move() /* Somewhat less wrong, indirectly */
-Shoot() /* Wrong */
-TakeDamage() /* Wrong */

Why would a building have health, be able to shoot, or take damage?
Why would a skybox have a skeleton?

You won’t understand what the single-responsibility principal means unless you understand what it means to have a single responsibility.

A CEntity is the lowest-level thing that gets into your game world. Every single object inherits from it. It has 1 job and 1 job only: Provide things with a parent/child relationship.

Next level up you have a CActor, which provides things with world/local orientations.

I didn’t mark your suggestion that an entity have a position wrong because an entity and actor are often combined into 1 and the line between them is ambiguous.


So the only thing every single object in your game world has in common is that they can be parented under each other and they have a local position, which gets translated into a world position whenever changed.
How could you give a health bar to every single object in your game? Does the ground have health? Do buildings explode if you punch them enough?
At this level, we haven’t even gotten to models yet.

CModelInstance -> CActor -> CEntity

Now we have an actual mesh, which can represent some physical objects in the scene.

CDrawableModelInstance -> CModelInstance -> CActor -> CEntity

Now we have physical objects that can be drawn. Remember, not every object is visible. Invisible walls obstruct your path just as well as any other objects do.


CPlayer -> CDestructable -> CDrawableModelInstance -> CModelInstance -> CActor -> CEntity
CGenerator -> CDestructable -> CDrawableModelInstance -> CModelInstance -> CActor -> CEntity
CGiantMoth -> CDestructable -> CDrawableModelInstance -> CModelInstance -> CActor -> CEntity

Now we have something that can take damage. And it took 6 classes to get there.
This is Single-Responsibility Principal. It means a class should do only when it needs to do. It may do more than one thing, of course, as long as it makes sense for it to do that. A CModelInstance can be animated and thus have a skeleton. Because we all know that things may move even if we can’t see them. It makes sense for animation to be part of CModelInstance rather than CDrawableModelInstance. And this is a balance between “is a” and “has a”.

This is Single-Responsibility Principal. Things may have more than one duty, but those duties are logically a part of the object, and may even be delegated to a member of that object. For example a CModelInstance introduces the concept of animation to an actor, but a CSkeleton is the class that actually performs everything related to animations.
Likewise, an actor does not handle the responsibility of moving just because it introduces the concept of a position into the mix. The position, scale, and rotation of anything in the world is handled by the COrientation class. A CActor may introduce local positions to objects, but managing those positions is not its responsibility—that falls to COrientation.

This is Single-Responsibility Principal.


L. Spiro

Edited by L. Spiro, 07 June 2013 - 04:08 PM.

It is amazing how often people try to be unique, and yet they are always trying to make others be like them. - L. Spiro 2011
I spent most of my life learning the courage it takes to go out and get what I want. Now that I have it, I am not sure exactly what it is that I want. - L. Spiro 2013
I went to my local Subway once to find some guy yelling at the staff. When someone finally came to take my order and asked, “May I help you?”, I replied, “Yeah, I’ll have one asshole to go.”
L. Spiro Engine: http://lspiroengine.com
L. Spiro Engine Forums: http://lspiroengine.com/forums

#6 Tispe   Members   -  Reputation: 1032

Like
1Likes
Like

Posted 07 June 2013 - 04:45 PM

I didn't know inheritance and SRP were so close. Can't you have SRP without inheritance?

 

Sorry for the bad naming, but when I think of an Entity I think of a living creature like a PC or NPC, which has a mesh, skeleton etc.

 

It seemed so obvious that SRP ment that you made classes that contained data and then you made classes that manipulate that data, and then a render class that draw that data. Why a PC and a Sound clip needs the same base class is not clear to me.



#7 Ravyne   GDNet+   -  Reputation: 7330

Like
3Likes
Like

Posted 07 June 2013 - 06:19 PM

Inheritance is just a tool that you use to express a particular kind of relationship between classes. Its neither indicative of, or necessary for, SRP to be maintained. There are many, many ways to express relationships and compose objects in C++.

 

SRP is really about how complex concepts are broken down into their most atomic parts -- the idea is that its much harder to understand and reason about a large, complex ball of code and data than it is to understand and reason about a functionally identical system that's made up of smaller, self-contained portions of that functionality. Additionally, because those smaller systems are now independent of one another, you can use them as the basis to build other complex objects easily.



#8 Khatharr   Crossbones+   -  Reputation: 3000

Like
1Likes
Like

Posted 07 June 2013 - 06:22 PM

Sorry for the bad naming, but when I think of an Entity I think of a living creature like a PC or NPC, which has a mesh, skeleton etc.

 

It seemed so obvious that SRP ment that you made classes that contained data and then you made classes that manipulate that data, and then a render class that draw that data. Why a PC and a Sound clip needs the same base class is not clear to me.

 

Entity usually refers to any interactive element within the game world, or something along those lines. PC/NPC could be classified as 'Actors'. The is-a relationship would be, "Player is-a Actor is-a Entity", so Actor could inherit from Entity and Player could inherit from Actor. Inheritance is its own bucket of worms, and should be studied separately and comprehensively in order to save you a lot of grief later on.

 

SRP does not mean that you have classes that are all data and classes that are all action. That's really not a great concept to bind yourself to either, as it sounds like a recipe for inappropriate intimacy and eventually a god class. Additionally, SRP does not apply only to classes. It also applies to functions, modules, and really any segregable part of a program.

 

Make simple parts, then use them to build other simple parts.

 

The real goal is that someone can look at one of your parts and easily understand what it does just by reading through it, without needing to look up how its component parts work, or understand how it fits into the larger system.


void hurrrrrrrr() {__asm sub [ebp+4],5;}

There are ten kinds of people in this world: those who understand binary and those who don't.

#9 Hodgman   Moderators   -  Reputation: 30353

Like
4Likes
Like

Posted 07 June 2013 - 11:34 PM

Now we have physical objects that can be drawn. Remember, not every object is visible. Invisible walls obstruct your path just as well as any other objects do.
CPlayer -> CDestructable -> CDrawableModelInstance -> CModelInstance -> CActor -> CEntity
CGenerator -> CDestructable -> CDrawableModelInstance -> CModelInstance -> CActor -> CEntity
CGiantMoth -> CDestructable -> CDrawableModelInstance -> CModelInstance -> CActor -> CEntity

I didn't know inheritance and SRP were so close. Can't you have SRP without inheritance?

Yes, IMHO, the above is an abuse of inheritance. It's using "is a" (inheritance) where "has a" (composition) would suffice. Standard OO doctorine is to default to composition and only use inheritance where it's actually required -- i.e. where you actually need to polymorphically operate on some abstract type through an interface -- so if ModelInstance isn't implementing an interface that it's inherited from Entity, then it shouldn't inherit from it.
 
The main symptom that appears from defaulting to inheritance over composition is that your code is much more rigid and less reusable. e.g. what if we want to have an invisible wall that is destructable? Destructable inherits from Drawable, which the wall is not, so now we need to duplicate some code an make an InvisibleDestructable...

With composition and the SRP, the Player class has the single responsibility of acting as the glue that makes all it's member components work together:

struct Entity { virtual void Update() = 0; virtual ~Entity(){} };//Entity is often something like this
struct Player : public Entity
{
  Destructable  health;
  Model         visual;
  RigidBody     physical;
  Transform     position;
  AmmoContainer inventory;
//implement entity interface:
  void Update() {...}
};

IMHO, I'd get rid of the Entity base class in the above example as well, as it's too broad of an abstraction...

Instead of using inheritance to implicitly link together all of the individual components, in a more flexible design you can create the inter-component bindings when you construct the composite object:

struct Player
{
  Player( PhysicsWorld& pw, VisualScene& vs )
  {
    health.SetOnDamage( bind(&Player::OnDamage, *this, _1) );

    physical.SetOutputTransform( &position );
    physical.SetOnTouch( bind(&Player::OnTouch, *this, _1) );
    pw.AddShape( physical );

    visual.SetInputTransform( &position );
    vs.AddModel( visual );
  }
  bool OnTouch( const TouchEvent& e )
  {
    if( health.OnTouch( e ) ) return true;
    if( inventory.OnTouch( e ) ) return true;
    return false;
  }
  void OnDamage( const DamageEvent& e )
  {
    if( e.healthRemaining <= 0 ) { ... }
  }
...
};

Edited by Hodgman, 07 June 2013 - 11:49 PM.


#10 IncidentRay   Members   -  Reputation: 154

Like
0Likes
Like

Posted 08 June 2013 - 01:37 AM

Just wondering, Hodgman, with this design at what point are the references to the Model and RigidBody components removed from the PhysicsWorld and VisualScene objects?  If the Player struct calls a remove method in its destructor, then the Player would need to store a reference/pointer to the PhysicsWorld and VisualScene, which seems a bit inelegant IMHO.  Or is storing these references actually the right way to do it?



#11 kunos   Crossbones+   -  Reputation: 2207

Like
0Likes
Like

Posted 08 June 2013 - 02:20 AM

methods:

-Move()

-Shoot()

-TakeDamage()

 

Theory is surely a nice thing.. but if implementing this SRP means that a class can't have 3 methods then I'd rather stay away from it :P


Stefano Casillo
Lead Programmer
TWITTER: @KunosStefano
AssettoCorsa - netKar PRO - Kunos Simulazioni

#12 phantom   Moderators   -  Reputation: 7261

Like
2Likes
Like

Posted 08 June 2013 - 04:41 AM

Yes, IMHO, the above is an abuse of inheritance. It's using "is a" (inheritance) where "has a" (composition) would suffice. Standard OO doctorine is to default to composition and only use inheritance where it's actually required -- i.e. where you actually need to polymorphically operate on some abstract type through an interface -- so if ModelInstance isn't implementing an interface that it's inherited from Entity, then it shouldn't inherit from it.

This.
1000x this.

Inheritance chains longer than 2 or 3 deep is likely a symptom that you've now got a horribly ridge relationship graph between all your classes and, as Hodgman goes on to say, some which are 90% duplicates of the others in order to get around a 'small problem'.

Small, focused, simple classes are not only easier to understand and compose but also have the side benefit of likely being a more friendly size for CPU caches as you won't have a cascade of data items playing a game of Take Up Cache Space during various updates etc.

#13 Waterlimon   Crossbones+   -  Reputation: 2562

Like
0Likes
Like

Posted 08 June 2013 - 04:41 AM

Just wondering, Hodgman, with this design at what point are the references to the Model and RigidBody components removed from the PhysicsWorld and VisualScene objects?  If the Player struct calls a remove method in its destructor, then the Player would need to store a reference/pointer to the PhysicsWorld and VisualScene, which seems a bit inelegant IMHO.  Or is storing these references actually the right way to do it?

*write tl;dr answer*

*come to ok solution*

 

Make all the 'services' (PhysicsWorld, VisualScene) contain the corresponding entities, which are simply dummy containers of other components or raw data if 'primitive'.

 

The services themselves have methods to modify the components through references, HOWEVER, the entities are accessed by IDs (so you can move them in memory without invalidating everything). You MIGHT want to overload the methods with versions that take in IDs, fetch the component, and call the reference taking version.

 

Then, you should add an iterator for each service. The iterator stores the entity ID we are edition, along with its reference, so we dont have to pass the entity ID/reference to the service for each operation

 

//not sure what to call this as it doesnt need to iterate, although it very well could.

auto iter=PhysicsWorld.getEntity(PhysicsEntityID) //Creates iter, stores ref. to actual physicsentity, stores the ID, stores ref to PhysicsWorld or something

iter.setVelocity(blah) //edits the actual physicsentity directly if possible, or tells physicsworld/whatever else is available to do it (like if it needs a service only physicsworld knows about to access a component of physicsentity

iter.addForce(bleh)

PhysicsWorld.destroyy(PhysicsEntityID)

PhysicsWorld.runDemCollisionDetections()

 

you could possibly do something like

 

PhysicsWorld.setVelocity(PhysicsEntityID,velocity)

 

too i guess.

 

This needs some improvements though, but i thought this might work.


o3o


#14 Juliean   GDNet+   -  Reputation: 2605

Like
1Likes
Like

Posted 08 June 2013 - 10:09 AM

Theory is surely a nice thing.. but if implementing this SRP means that a class can't have 3 methods then I'd rather stay away from it tongue.png

 

Thats not what SRP say - it means that a class should have only methods that aid into solving a particular problem. From what I know SRP should also be applied to methods themself, so you could actually end up with a lot of small methods in one class, as long as they are necessary to solve a particular problem (parsing an xml file, ...). Moving, shooting and taking damage are independant problems, thus they should be seperated into smaller classes following SRP.


Edited by Juliean, 08 June 2013 - 10:10 AM.


#15 L. Spiro   Crossbones+   -  Reputation: 13576

Like
-1Likes
Like

Posted 09 June 2013 - 03:05 AM

Hodgman, on 08 Jun 2013 - 14:25, said:
Yes, IMHO, the above is an abuse of inheritance. It's using "is a" (inheritance) where "has a" (composition) would suffice. Standard OO doctorine is to default to composition and only use inheritance where it's actually required -- i.e. where you actually need to polymorphically operate on some abstract type through an interface -- so if ModelInstance isn't implementing an interface that it's inherited from Entity, then it shouldn't inherit from it.

It does, although I did not go into so much detail.
CEntity provides the basic parent-child relationship and I mentioned that CActor and CEntity are often combined into one, though I chose to separate them for cleanliness (again, basically—not going into too much detail). I’d fault no one for combining them (as I have suggested multiple times on this forum and in my book). Making an efficient parent-child relationship with dirty flags that need to be cascaded down to children when local or world translations/scales/rotations change is fairly complicated (when unnecessary updates are to be completely eliminated), further complicated by the “WillChange”/“DidChange” mentioned below, so I made a personal choice to keep them separate.

But what I did not mention is that CActor provides “WorldMatrixWillChange()”, “WorldMatrixDidChange()”, “LocalMatrixWillChange()”, and “LocalMatrixDidChange()” virtual functions that upper classes do actually override, including CModelInstance.

Why? Because CActor/CEntity know nothing about what a bounding box or bounding sphere is. A CModelInstance does, and it needs to be updated when the world matrix changes. That is to say, “CModelInstance implements an interface provided by CActor”. This is proper use of “is a/has a”.
Likewise, a CDrawableModelInstance is a CModelInstance, with the ability to render itself, and yes it does implement an interface provided by CModelInstance, but again I will not go into so much detail.


The rest about CDestructable and CPlayer was off the top of my head to provide an example of what might happen on the game side of things. Everyone should put thought into his or her design, and that example may not be suitable for everyone, but it was just an example.

I also mentioned the balance between “is a/has a”, citing COrientation as an example.

It is definitely debatable as to how CPlayer and CDestructable could be organized, although such a debate would be pointless as there could never actually be a winner. I would be perfectly fine with composition in that case despite my example using inheritance, and admittedly it makes more sense that a player “has a” drawable component and a destructable component.
But to say that inheritance chains deeper than 2 or 3 is a symptom of rigid design and an attempt to get around a small problem is an extreme over-generalization.


L. Spiro


Edited by L. Spiro, 10 June 2013 - 05:52 PM.

It is amazing how often people try to be unique, and yet they are always trying to make others be like them. - L. Spiro 2011
I spent most of my life learning the courage it takes to go out and get what I want. Now that I have it, I am not sure exactly what it is that I want. - L. Spiro 2013
I went to my local Subway once to find some guy yelling at the staff. When someone finally came to take my order and asked, “May I help you?”, I replied, “Yeah, I’ll have one asshole to go.”
L. Spiro Engine: http://lspiroengine.com
L. Spiro Engine Forums: http://lspiroengine.com/forums




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS