Advice on Unity3D SOLID SRP

Question

I'm currently making a game in Unity3D and am trying to implement the Single-Responsibility Principle of SOLID into my game as I'm learning it.

I was wondering if anyone could explain a little more on what a good implementation of SRP looks like because I feel like I'm breaking it.

I've broken down my classes into the basics parts:

• PlayerInput.cs - Gets Input an stores them into variables (Horizontal, Vertical, RMB, etc)
• PlayerController.cs - Handles attacks and player states (idle, moving, attacking) ATM
• PlayerMovement.cs - Handles movement and rotation (look at mouse)
• AnimationController.cs - Handles All Animations

When I started I felt I was following SRP but now the game is getting more complex. Each class listed above gets a reference to the others which seems unnecessary. I'm using like 5 GetComponents in each class and it seems repetitive because they are all on the same object. In other words SRP seems like more work and is making it less efficient.

For example, both the PlayerController and the PlayerMovement script have a reference to AnimationController where AnimationController has a reference to both as well. All of the scripts have a reference to PlayerInput. (Keep in mind I'm leaving out other player related scripts like crafting and equipment to keep this simple but my Start and Awake methods are full of a bunch of GetComponent calls)

Could anyone explain SRP to me better and maybe point me in the right direction so I'm not using GetComponent so much on the same GameObject.

Thanks

Answer 1

You already correctly identified the problem, which obviously needs to be solved regardless of theoretical principles. But ok, let's discuss SRP first:

The SRP refers two older concepts namely coupling and cohesion. Unfortunately the name "SRP", is confusing, ambiguous and only contains one side of the equation, which is to split stuff. It doesn't mention that stuff the belongs together should be actually together.

So the point of all this is to enable maintainability, which is a shorthand for creating less future work. To do this it seems reasonable that things that refer to eachother should be actually together. This means methods that work with some data should be co-located with that data (i.e. in the same object). Stuff that is loosely related (i.e. refer to each other rarely) should be split.

How to do this in practice depends on the business case and is not always apparent. I suggest an exercise. Put these things together in one class, call it Player for example. Simplify it, remove all setter/getters and indirections. Then try to see if there are areas that only loosely refer to each other, or refer only in one direction. These will be good candidates to split out.

Try to split up meaningful things instead of technical ones, i.e. Movement, Attack, Player are all good, Controller, Animation are questionable (though not always bad).

Answer 2

It doesn't sound like what you're referring to is related to the SRP. At its most basic, the SRP means that any class you have should only be "responsible" for one thing. Your PlayerInput is responsible for tracking what the player has provided through their interface devices, PlayerController provides stateful information about the player based on a number of factors, and so on and so forth.

What you seem to be referring to is more related to the DRY (don't repeat yourself) principle. DRY is great, and it provides a great backbone for abstraction and code reuse, but it isn't gospel. In fact, one of the SOLID principles - the dependency inversion principle - in some ways actively encourages one to repeat oneself in the name of flexibility.

If you think you're calling a method too much, you could consider wrapping a series of calls to that method and any interceding logic in another method and calling that instead. This, of course, depends on that same set of instructions needing to be called in multiple places.

Now, I will say that one part of your post that jumps out is cyclic references between yourAnimationController and, respectively, your PlayerController, and PlayerMovement classes. This is actually a violation of the SRP and a few other principles of good design, as the enclosed classes are now responsible for their assigned task as well as, at the very least, keeping track of what's going on in the AnimationController, and at the very most, calling the methods of the AnimationController itself.

You should consider refactoring your more granular classes such that they are able to do their jobs regardless of what's going on in the AnimationController, or vice-versa. The enclosing class should handle the logic to tell the enclosed class what to do, essentially making the decisions for the enclosed class.