I understood @CodeSmile s comment about Manager class code smell as directed only against naming classes in such a way. And I agree with him, and with the suggested name replacements, I use a similar approach myself.
By code smell it is meant that “this is possibly a place of bad design also, as the naming is so bad”.
It’s a good point that my refactoring example was probably taking things further than what @CodeSmile was suggesting.
My main point is I personally haven’t found the use of manager classes to be an issue in practice, so I don’t personally see that word as a red flag.
But I do agree with the sentiment that simple and exact names are good. I really like it when an API is pretty much like reading correctly spelled English: Player.Killed, Inventory.GetItem etc.
It’s a good class(but PlayerHealthChanged ?). i was talking about “Manager” with > 300 lines of code
It’s about Clean Code and the idea that time spent to naming things is a part of the design process. XXXManager is the first word that comes to mind, but the idea is to spend a bit more time thinking about the names. This will improve readability of code but it also can improve design because that name says nothing about the actual purpose of the class. When the name is too generic, “GeneralManager”, it sort of invites you to lump functionalities there that could be separated into separate aspects, and your manager can also grow too big in size.
So the take-home from this would be to keep the manager classes smaller in size and more specific to purpose.
I totally agree that spending time and effort to optimize names is very important.
I think where my opinion starts to deviate a tiny bit, is in whether or not adding the -Manager suffix to class names can be more optimal than not.
In a project with good architecture, I believe it should ideally be trivially easy to figure out where new code should be added when new functionality is needed. Grouping classes under generic, high-level categories like “managers” can help give some guidance in this regard, in my experience. When you’re working in a code base with thousands of classes, stuff like this can matter quite a bit.
In general I think that class names should be optimized a lot for discoverability and intuitiveness, while method names should be optimized a lot for definiteness.
With this I agree 100% 
Point is, the suffix doesn’t really add information. I think it’s a remnant from SOA ages where it sounded cool to name things “Managers” because it highlighted that this is a service. It was cool that it was a service and not just a monolith subroutine, and you could call it from outside.
To me the -Manager suffix on something like PlayerManager does imply quite a bit of information:
In addition to this, the word “manager” can encompass a lot more information within a particular project. You can add a summary to a base Manager class, and after a new developer joins the project, they can read this one summary, and immediately gain information relevant to dozens of classes in the codebase. For example, perhaps an instance of any Manager-derived class can automatically be retrieved from a service locator.
A very similar example is the concept of “systems” in ECS. As a word “system” is also very high-level and nonspecific. But at the same time, in the context of the ECS package, it is an extremely useful concept to have for a lot of reasons, and I don’t think that adding the suffix -System to various classes in a project is a code smell.
Some uncollected thoughts:
Could rename WarriorFactory.CreateWarrior to just Create, because the type of the created object is already apparent from the name of the class (and more exact too from the name of the concrete class).
Could rename AbstractWarrior to just Warrior for simplicity and brevity.
Could wrap WarriorFactory members in a class like WarriorStats, and then remove _maxHealth, _maxStamina, _weight, _strength from AbstractWarrior, and instead just pass in a reference to WarriorStats in the constructor (flyweight pattern).
AbstractWarrior.TakedDamage could be renamed to Damaged to be more consistent with the naming of the other events (and fix a mispelling).
The abstractions IDamage, IAttack, IHealth feel off to me. You can now damage a warrior through any of these three interfaces - so which one should be used in which situation? Perhaps something more like this would be more intuitive:
public interface IAttackable
{
bool CanBeAttackedWith(Weapon weapon);
void Attack(Weapon weapon);
}
public interface IKillable
{
bool IsAlive { get; }
float CurrentHealth { get; set; }
float MaxHealth { get; }
void Kill();
}
There are quite a few abstract classes related to one warrior entity: AbstractWarrior, WarriorFactory, WarriorView, WarriorPresenter, WarriorMovement. It feels like adding additional warriors using this system could be quite arduous and lead to code duplication. Using composition more instead of inheritance could potentially help avoid this. In Unity functionality is most commonly composed out of multiple components (attach Attackable component to an entity to make it attackable, attach Killable component to make it killable, attach Rigidbody component to give it a mass etc.).
Consider making the Weapon class derive from ScriptableObject, to make it easier to create new weapons without any code modifications, and to tweak their stats on-the-fly during playtesting.
The WarriorPresenter abstraction also feels weird to me. It’s called a “presenter”, but then it handles functionality like stamina regeneration, movement, jumping and updating UI views. I would try to get rid of this abstraction completely, and make all those different unrelated functionalities be handled independently by individual components (e.g. StatRegenerator, MoveHandler, JumpHandler).
There’s a class called KnightMono that derives from WarriorPresenter, which feels inconsistent; I’d expect all the derived classes to have the same suffix as the base class.
Agree, it looks more reasonable.
IDamage - for entity which can take damage(enemies, player, destructable objects, etc…)
IAttack - for entities that can attack other objects
IHealth - for entities that has health(player, enemies)
I separate IHealth and IDamage because they have different purposes : for example when player takes damage he can lose stamina,etc… so IHealth here responsible only for health.
WarriorPresenter here is P from MVP : it presents AbstractWarrior(Model) to Unity(View).
Using composition instead of inheritance sounds good, i will try it. Thanks for clarification
I read through your list and didn’t find anything to promote naming your class “Manager”.
You don’t want any hierarchy to your game but events. This suits games particularly well. Your game responds to events. You have e.g. “Weather”… what it does is schedules and publishes weather events. Whomever needs to respond to weather events, listen to Weather.
Your Player input consists of events. Player hit Menu key? Send event, and your GUI listens to it. Et cetera. These of course are a built-in platform feature.
Your character is a huge source of events, stuff happens to him all the time! He changes movement, collides etc. All of these are events. Character hit the wall? Send event. Character got hungry? Send event. Pro character controllers have it already.
EnemySpawner spawned an enemy? Send event. Enemy killed? Send event.
All of this just with publish-subscribe pattern, and you’re good.
The manager is all present and all knowing. It tells you when you’re doing good …or bad. And it tells you when your time is up.
Do not disrespect the manager!.
Because there’s no game without it…
You could also achieve the same effect with simpler components that raise events:
public sealed class Attackable : MonoBehaviour, IAttackable
{
public event Action<Weapon> Attacked;
public bool CanBeAttackedWith(Weapon weapon)
{
foreach(var attackabilityAffector in GetComponents<IAttackabilityAffector>())
{
if(!attackabilityAffector.CanBeAttackedWith(weapon))
{
return false;
}
}
return true;
}
public void Attack(Weapon weapon) => Attacked?.Invoke(weapon);
}
public sealed class Killable : MonoBehaviour, IKillable, IAttackabilityAffector
{
public event Action Died;
public event Action<float> Damaged;
bool IsAlive => _currentHealth > 0f;
bool IAttackabilityAffector.CanBeAttackedWith(Weapon weapon) => IsAlive;
public float CurrentHealth
{
get => _currentHealth;
set
{
value = Mathf.Clamp(value, 0f, MaxHealth);
if(_currentHealth == value)
{
return;
}
float damageTaken = _currentHealth - value;
_currentHealth = value;
Damaged?.Invoke(damageTaken);
if(value <= 0f)
{
Kill();
}
}
}
public void Kill() => Died?.Invoke();
private void OnEnable()
{
if(TryGetComponent(out _attackable))
{
attackable.Attacked += OnAttacked;
}
}
private void OnDisable()
{
if(_attackable != null)
{
attackable.Attacked -= OnAttacked;
}
}
private void OnAttacked(Weapon weapon) => CurrentHealth -= weapon.Damage;
}
These events could then be used to trigger different kinds of components that are attached to the same GameObject. For example:
public sealed class OnDamagedAdjustStamina: MonoBehaviour
{
[SerializeField] private Killable _killable;
[SerializeField] private Stamina _stamina;
[SerializeField] float _adjustAmount = -10f;
private void OnEnable() => _killable.Damaged += OnDamaged;
private void OnDisable() => _killable.Damaged -= OnDamaged;
private void OnDamaged(float damageTaken) => _stamina.CurrentStamina += _adjustAmount;
}
You can also use UnityEvents to make it possible to hook the events up to any methods via the inspector (this is very powerful, but can also be quite fragile, with serialized data easily breaking when methods are renamed for example).
public sealed class OnDamagedIntTrigger : MonoBehaviour
{
[SerializeField] private Killable _killable;
[SerializeField] private UnityEvent<int> _onDamaged;
private void OnEnable() => _killable.Damaged += _onDamaged.Invoke;
private void OnDisable() => _killable.Damaged -= _onDamaged.Invoke;
}
My worry with having three different methods for dealing damage to the warrior is that it could be very easy to forget to raise all the relevant events in the same order when damage is dealt through each of them.
Ah, I see… then I would just remove the WarriorView class completely, and just hook WarriorPresenter directly to the Image components. I would consider the Image component to already be a View.
After this change it makes sense to me for WarriorPresenter to act as a mediator between the Warrior and its views (images, texts, buttons…).
(I would personally maybe also rename the class to something like WarriorUI for simplicity, but that’s just me 
)
Why hook up events to methods? Events are not messages. They don’t travel, they just occur. Just write the events to your topics/queues and let anybody subscribe to listen who wants to.
Event data is serializable game data but luckily your events won’t get too coupled and also you don’t even need to carry much data in the events because you don’t want to restrict your component visibility to one another. So, if your Player GUI part is triggered with “Player movement style changed” update from the event topic, this code can read the Player class to get the data that he needs. The important bit was that he knows he needs to update the GUI cos the event occurred and now’s the time to do it.
Any changes to saved serialized data needs save file version increase.
If you don’t know how to build this, you need some sort of event broker system for it. I myself use the one that comes with Opsive character controller, but any proper one will do that supports publish-subscribe and doesn’t blow up. I think there’s event systems on Asset Store or you can build your own. I was using my self-made one before switching to this other one cos I happened to have it. You can build it with Unity Events or without them. Opsive is using a DLL which is supposed to load GC less.
If an event is raised and no one is around to hear it, does it make a sound?
You do also want to hook up subscribers to the events - otherwise why raise them in the first place?
You don’t hook them up, they subscribe. Now or at a later stage. In designing the events, an event is described as “something significant happened” and they should be a part of your game design, whether someone listens or not.
Want to make an on-screen event log? Just hook it up. Want to build a second UI? Just hook it up. You don’t need inspector for this, just a single line of code, typically in Start or Awake. And you cause no side effects so you don’t need to control it manually. The broker handles the coupling.
I’m a huge fan of pub/sub (broker) and rarely use events. I use this library Pub-Sub Messenger | Utilities Tools | Unity Asset Store
This is specifically about programming a game, right? And we want to be efficent, right?
Then we should look at what is the core of programming any game, what takes the most time:
Experimentation and Production. Those two often go hand in hand.
Production means iteration, you add a new enemy, you change it, you remove it. A new weapon, a new combo attack, a new item with a very special behaviour. To be able to do this without ending up with a huge plate of spaghetti your code has to be flexible.
(except the game is super simple, like hyper casual, a game jam game, walking simulator, … but as you are worried about code architecture, I expect your games to have some degree of complexity)
Flexible Code = Efficient Code for Games. And to make it flexible, you have to decouple it, so you can add/change/remove features without rewriting (or even affecting) 90% of the game each time.
Decoupling always works in the same way: instead of Connecting two classes (A, B) like so:
**A <-> B**
you add something in between, like so:
A <-> X <-> B
X can be an interface, a seperate class, a scriptable object, an event system, even reflection or a text file (save game).
What you use is entirely up to you, there is no silver bullet, I use pretty much all of them.
Also there are some established patterns for certain use cases. As previously mentioned, the PubSub-Pattern is popular when it comes to things like “sending damage to an enemy”. Statemachines are used for Enemy-AI & Player-Character-Controllers (sometimes even for menus). Component-Pattern for a list of weapons (all weapons deriving from WeaponBase, referenced in WeaponController which enables/disables them for example). The list goes on and on.
The core is that you want flexible, decoupled code so you don’t end up with Frankenstein-Classes which you basically have to rewrite entirely whenever you have to fix a bug or add/change/remove a feature.
Btw. I’m working on a video on this topic since over one year, which hopefully should be released soon, link to my youtube channel is in my signature - there I will explain all of this in greater detail with a ton of animations (making those text-animations (highlighting code and stuff) is driving me insane but I’ll push through it eventually)
I like your description of decoupling. You have to put something in between. We would even probably call that thing a coupler in some cases in traditional physical engineering. To me the issue is whether that thing is simple or abstract enough for universal application, and whether it is widely adopted, just like adapters in the real world.
It’s not at all as simple as just putting something in between. Event-based architecture is very different from interfaces, they serve a different purpose.
If you use an interface, you are basically creating a contract between two modules about how these modules act in relation to one another. However, in event-based architecture your new module just adds to the event stream and makes itself usable. So, rather than putting two things together and something in between, in event-based architecture you have a grid, and by bringing in a new module, you are adding something to the grid that produces a new event stream and can be used by any other modules. Of course, if you then unplug your module, those modules that use its event stream won’t work anymore, but they won’t break, and you don’t need code changes. They just don’t get input anymore.
I personally don’t use interfaces at all in Unity and I haven’t found a place where I’d need them. I use only abstract classes. This is because interfaces are rigid contracts, and development just rarely goes the way that first you design the interface, then everybody follows that. Most of the time you are experimenting and the rest of the time you are cleaning up.