Single responsibility principle - Is it bad to use a single access point for all classes?

Unity Engine
1

I am making a game, in this game I have units. I used to just have one big class called Unit which did more or less everything, which is bad.

So I split it up into several classes, UnitSelectable, UnitAnimator, UnitDetection, UnitAi, UnitHealth, UnitDataContainer, and so on…

Now to the problem, I am using a State Machine where I want states to be quite generic, so they just take a parameter (Unit unit), then I can do whatever I want in the State using this. But now that I have a ton of classes managing the behaviour of a Unit, so what do I pass to my State? What if I want to do something in the state that needs UnitAnimator and UnitHealth?

Simply passing a GameObject to my StateMachine would require me to use GetComponent<> a lot, which seems like quite bad for performance, or?.

So to my question, would it be bad to create a class Unit that simply holds a reference to all other classes above, then I can pass that Unit to my state machine and do whatever I want in there?

2

What you’re describing is an adapter. This is a perfectly good solution to your specific problem. However, I suggest the adapter do more than just hold references to your other components. It should expose an interface to your state machine that makes sense for what the state machine needs to accomplish. For instance, rather than a property called UnitAnimator which the SM “dots into”, just create a method like PlayAnimation which will use UnitAnimator in the background but insulate the SM from that actual class.

When building your adapter, don’t try to anticipate all the things your SM will want to do. Just add the methods organically as you find it needs to do something else. Also, don’t start by putting all the Unit components into the adapter, just add them as you find you need them. Occasionally, review the methods and Unit components you have added and consider how you might refactor the adapter to reduce the number of methods or to split into to seperate adapters.

1 Like
3

Thanks, could you exaplain:

It should expose an interface to your state machine that makes sense for what the state machine needs to accomplish.

What do you mean by exposing an interface?

Also, the more I think about it, maybe it is better my StateMachine just takes a GameObject instead, that way I can use it for more than Units. While the drawback is that it will lead to more GetComponent<> calls, I can still avoid it in Update.

I can cache GetComponent<> calls in Enter that I need in Execute, does that seem like a good idea? :slight_smile: It feels more like the “Unity-way”, if that makes sense.

public interface IState
{
    void Enter();
    void Execute();
    void Exit();
}
4

All objects expose an interface: it’s just the collection of all their public methods. I know C# has a keyword interface, so that can confuse matters. In all OO languages, every type has an interface, whether explicit, as in C#, or implicit. When I say “expose an interface”, I simply mean, make public methods that are useful to the client of this type (the state machine in your case)

Yes, you could do that. You could also just use System.Object. Then could pass anything at all. However, I don’t think I need to explain why that isn’t as good a solution. Wherever possible, we want to limit the types we pass to the most specific type possible. This limits our dependencies on external objects and reduces the likelihood of rippling changes in the future. This is one of the goals of the Liskov Substitution principle.

Yes, you should always cache GetComponent calls if you plan to use the component on a regular basis. The call is quite slow.

1 Like
5

Thank you!
Okay so, lets say I use an adapter, mediator, whatever you want to call it.

If I do as you say and I let the adapter do more than just hold references to your other components, I can see my self quickly ending up with a really large adapter-class, which is what I was trying to get away from.

What makes most sense in my head is simply using the Adapter as a cache for my components, i.e:

public UnitAnimator unitAnimator;
public UnitHealth unitHealth;
//...

This way I can even set them in the inspector. The advantages I see are:

  • It follows the Single Responsibility Principle, which I didnt before.

  • I don’t have to call GetComponent<> more than I used to, since everything is cached.

  • I can pass the adapter to my sm, and simply call unitAdapter.unitAnimator.PlayAnim();

Is there a reason for the above approach being bad? Somehow it feels like Im fighting the engine here.

6

This sounds like a symptom of doing too much in your SM, which is probably not following the SR rule. Or, perhaps you are trying to hard to keep a single abstraction “Unit” for the SM.

There will be many ways to accomplish what you are doing. Programing is the art of predicting the advantages/disadvantages of the various approaches and choosing the one most appropriate to your situation. If you’re happy with those advantages, then you should take that approach. Since we don’t have as many details as you, the best we can do is point out where you stray from patterns we’re familiar with. Sometimes it makes sense to do that.

It’s fine, but to me it sounds like an anemic class.

7

This is probably true, my SM does more or less everything, move, attack, gather, build, idle, and so on.

I learned a lot from this discussion so thank you very much. I’ll keep testing stuff out, and trying to learn.

1 Like