single responsibility principle in practice

Started by
13 comments, last by L. Spiro 10 years, 11 months ago

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
TWITTER: [twitter]KunosStefano[/twitter]
AssettoCorsa - netKar PRO - Kunos Simulazioni

Advertisement

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.

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

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.

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

I restore Nintendo 64 video-game OST’s into HD! https://www.youtube.com/channel/UCCtX_wedtZ5BoyQBXEhnVZw/playlists?view=1&sort=lad&flow=grid

This topic is closed to new replies.

Advertisement