So the project is almost at the end of development, it's gonna get shipped in matter of 2-3 months, everything is working quite fine and the amount of “Manager” classes are now up to 24 - is it too many? What would be alternative to this? I like small classes rather than having GameManager with 10k lines and I didnt really encounter any issue with it along 2 years of project development with this approach. What's your thoughts?
Thanks and have a good one!
Why do these things all need a “manager” class? I have no idea from the name what these classes are even supposed to do.
I can imagine pretty well what the classes do, and i don't think it's too many.
But i could not tell if there is something like a major idea on what's good practice and what not. If it works for you, it should be fine anyway.
Jikoob said:
So the project is almost at the end of development, it's gonna get shipped in matter of 2-3 months, everything is working quite fine
Looks like the most important metric is good.
Jikoob said:
the amount of “Manager” classes are now up to 24 - is it too many? What would be alternative to this?
Looks like naming conventions to me. What is their SINGLE responsibility? Could they be a builder, reader, writer, handler, container, target, controller, model, view, factory, pool, cache, session, provider, service, editor, adapter, or something else, then that might be a better name.
If it is working and people are used to the names, changing the name at this point is more bother than it is worth. Very often it is good to review names early in object designs, double-check what their single responsibility is, and find a title that focuses on that responsibility. Classes with too much functionality (often called “god classes”) are usually better once they're decomposed into smaller classes that each have more focused responsibilities.
Still, if the game is getting ready to ship just live with it. Pick better names in the next project.
and the amount of “Manager” classes are now up to 24 - is it too many? […] I like small classes rather than having GameManager with 10k lines
Generally, I would say that most developers would agree with you - it's better to divide code by functionality, than to have one god class that is hard to read, reason about, and modify.
That's why "god class" or “blob” is an antipattern, and “Separation of concerns”, “Single responsibility principle”, or “Modular programming” are viewed favorably.
What would be alternative to this
So, if we agree that it's better to split classes so each has a single responsibility, it's up to a philosophical discussion on how to do things in unity.
I would generally avoid the creation of prefabs/monobehaviours per “manager” class, and just create a “plain” C# singleton, if possible. (or move all your managers into one single parent prefab, if you want to have them serialized)
Kaosumaru said:
I would generally avoid the creation of prefabs/monobehaviours per “manager” class, and just create a “plain” C# singleton, if possible. (or move all your managers into one single parent prefab, if you want to have them serialized)
They're all under P_#ESSENTIALS Prefab.
When it comes to plain C# classes, this is something I'd like to try out in the next project - I want to test out an architecture that is somewhat based on dependency injection with composition root and basic application entry point.
The only thing that feels awkward with approach presented in the thread is the dependencies management and overuse of singleton pattern which forces concrete types references - It's an issue that touches not only managers but the whole project.
What I'd like to try is to have a static Engine class which is the entry point of an application. The Engine class would initialize the game by creating instances of all the necessary managers/controllers and keep references to those, injecting them where needed. If anything in the project needs a reference, for example, to AudioManager, I will no longer have to reference the concrete singleton instance of the AudioManager class, but rather ask the Engine to give me the IAudioManager, which allows for different implementations thanks to interface use. Each Manager would then have a Configuration or Settings class that would most likely be a ScriptableObject, since editor serialization would not be available.
This way, I will no longer need singletons, there would be close to a minimum of editor "drag and drop" references, the service locator pattern would actually be useful, since Unity Find<>() functions are just too heavy, and on top of that, I'll be able to eliminate most race conditions since the initialization chain would be easy to read and maintain. Additionally, designers wouldn't have to change serialized properties in the scene but rather in the configuration/settings file, and the scenes would contain content/assets only.
The settings as scriptable objects is indeed a very good pattern.
Not only you don't have to fiddle with scenes, the changes will be persistent (i.e. stop play, lose adjustments).
One extra thing I did is to further split settings as N scriptable objects, one can selectively replace parts live:
Jikoob said:
singleton pattern which forces concrete types references
Technically, this generally isn't an issue with Singleton. Singleton is sometimes frowned upon, cause it creates problems with unit tests, and is kinda like an implicit dependency. But basically, it boils down to “There is a globally available single instance of a given object”, and nothing forbids you from returning base type or an interface instead of a specialized class. (though tbh I usually dislike the creation of interfaces that are only fulfilled by one class).
So, if you have a static Engine class, and it provides you with an interface to a manager, like this “Engine.AudioManager.Mute()” - I would say that it's still a Singleton pattern. Difference between “Engine.AudioManager.Mute()” vs “AudioManager.Instance.Mute()” is only in semantics, not in logic, and it's not dependency injection if “Engine.AudioManager.Mute()” would be globally available (maybe I'm wrong, but I understood that it's your idea)
I think it's a good architecture anyway, I'm doing similar things myself.
I would say that it's still a Singleton pattern. Difference between … is only in semantics, not in logic, and it's not dependency injection if “Engine.AudioManager.Mute()” would be globally available (maybe I'm wrong, but I understood that it's your idea)
Yes, it is something different.
The Singleton pattern means something specific. Go look up the GoF definition for the details. It is a pattern that specified both the access AND the creation, in a way that prohibits many actions.
What you described is a C# syntax that allows some of the same access pattern but doesn't require the creation pattern. Generally that use is not a Singleton pattern, simply having a single static instance. A static instance is not itself a Singleton pattern because of the reasons you mentioned, you could replace it with a subclass and you could create other instances. It is the difference between “there is one” vs “there can only ever be one, this specific one”.
The Singleton pattern generally has a private constructor and is explicitly not virtual nor derived from a base with virtual methods, so it can't be subclasssed nor instantiated.
Kaosumaru said:
But basically, it boils down to “There is a globally available single instance of a given object”
That's true, but the difference is in the creation of the instance and how you access it - Default unity singleton implementation handles its own creation which may be troublesome when you deal with access via interface.
Handling instances creation in root Engine class gives you more control since you can create new instances in specified order/steps (builder pattern, no race conditions) - which also gives you the possibility to introduce dependency injection. Engine class would also decide which concrete type should be instantiated if it's being accessed via interface.
Kaosumaru said:
I usually dislike the creation of interfaces that are only fulfilled by one class
I like to stick to the rules to make things more readable - if any service/manager would be accessible via Interface, then every service/manager should be accessible via Interface, just to keep things organized. Some would say its an overkill but hey I like it that way.
For example I can't imagine how AchievementsManager could not be hidden behind interface since achievements API calls are different on all platforms. Although you could argue that you can use strategy pattern so that AchievementsManager would be static and it could choose which implementation of IAchievementsHandler to use in order to handle achievements for different platforms, so there's always a way out.
Kaosumaru said:
So, if you have a static Engine class, and it provides you with an interface to a manager, like this “Engine.AudioManager.Mute()” - I would say that it's still a Singleton pattern. Difference between “Engine.AudioManager.Mute()” vs “AudioManager.Instance.Mute()” is only in semantics, not in logic.
It would be more like "Engine.GetManager<IAudioManager>().Mute()" vs “AudioManager.Instance.Mute()”.
But if IAudioManager is a private static field in Engine, then its still just accessing static instance but with additional steps, so what's the deal? I think it gives us some more possibilities, the Engine can decide what's the concrete implementation behind IAudioManager. Also IAudioManager does not have to be static if you happen to hate singletons - The Engine can decide if it can create more than one of IAudioManager and how to pass it.
Kaosumaru said:
and it's not dependency injection if “Engine.AudioManager.Mute()” would be globally available
Yes, if you access it globally then it's not dependency injection, but if you treat the Engine as composition root and create new instances of all managers and other stuff, you can pass quite a lot of dependencies via injection at the initialization stage.
For example, if an Enemy needs a reference to IAudioManager, then:
Engine.CreateManager IAudioManager → Engine.CreateManager IEnemyManager(IAudioManager) → IEnemyManager.SpawnEnemy(IAudioManager) → Enemy.Initialize(IAudioManager)
If you stick to the plan, you can minimize the global access via service locator pattern to minimum or even ditch the global GetManager<IManager>() functionality and try to pass everything via dependency injection.
But then the IEnemyManager needs a reference to IAudioManager only because the Enemy will need it and it seems stupid so I don't know what think about it. And only solutions that come to my mind are service locator - Global GetManager<>() and Dependency Injection frameworks like Zenject that use DI containers which I dont know if I like or not.
