Code Quality Chronicles Vol 3: A handful of practices #Unity
As the codebase grows, pains of bad practices accumulate. The fact that production gets slower with more code, is a sure sign there are abundant bad practices in the project. Eliminating all the impedance - the crud - will increase the speed to the point where production speed accumulates. Although counter-intuitive, this speedup comes from code re-use and established working pipeline.
Some I would like to share right now:
- All Component references should be cached. The only other place where GetComponent is allowed is just after instantiating a prefab and needing to set now appropriate values (since there is no other way). Always cache that transform and WaitForSeconds.
- Pre-cache everything, especially strings. Don’t allocate at runtime if at all possible.
- Pool where appropriate.
- Be aware of performance costs of every move you make. I still use LINQ sometimes, and still write efficient code. If you’re just starting out, prefix all your methods that are called from Update_... you will quickly realize you’re probably allocating in Update.
- Have code on the same logical level, with clear separation of concerns. If your health script is setting the UI slider you’re doing it wrong.
- Never swallow errors or pretend to handle them gracefully by avoiding affected code. If something is not assigned in the inspector, we want to know right away.
- Use events. Don’t allow bi-directional GetComponent links if possible, this is a sign Observer pattern is needed. If you don’t know why you’d use events over a method call, you’re doing it wrong. It’s all about who knows about whom.
Conventions Of Practice:
- Singletons are a-OK. In general software development Singletons are regarded an anti-pattern but with careful analysis of pros and cons in game dev the conclusion is different. Even people are not able to manage themselves, they need managers. Further, in Unity all managers must be singletons, and self-instantiable ones. This means if the Singleton is not already in the scene, it will create itself with default values on first access. This is the only way to have self-contained prefabs, meaning if we put a prefab in any scene it will not cause NullRefException, instead it will initialize all the required managers by accessing them. This way any prefab will run with realistic behavior even when thrown in an empty scene. This means we can't allow scene references on prefabs (see below).
- Scene-only object references on prefabs are not allowed. Scene object to scene object references also. This workflow will cause all kinds of grief, people do it for convenience and then they wonder why everything seems to be linked to everything else and why they have trouble when they duplicate the scene. Use a Registrator instead, if necessary. Only ways of fetching data in an MB from the other objects: GetComponent or references on the same prefab, GetComponent or references on the same GameObject, asking their self-instantiable manager classes or asking a Registrator. Registrator is GO that only stores references to scene-only objects in one place. Registrators are different in that they may not be actually present, so the component must be able to gracefully continue if so, with a message. NOTE: You may argue that Registrators violate encapsulation. They do, but are worth it in practice.
- Never reference non-root go's of a prefab, instead use a Façade Pattern.
- Never set value to public fields in another class, that’s unexpected (except for injection purposes). Calling methods or using properties is expected. No point in using [SerializeField] private since the encapsulation is violated anyway. In fact, even private fields can be got through reflection, so no protection is ultimate.
Critique Of The Current Component System:
- Start and other „magic methods“ can be confusing. For example, enabling an object and accessing it's components right after it will not call Awake() or Start() before that, which now makes you access an object in a potentially invalid state. Instead, in these cases we use OnEnable, however this messes up the assumed order of Awake, OnEnable, Start if the object started disabled.
- Some magic methods are unexpected. For example, Reset() is such a method. Instead, magic methods should be prefixed for convention.
- Relationships with Coroutines and enable/disable are strange.
- Physix collision rules are strange. Having a limiting amount of layers constrains the design space.
- Having only 1 tag per GO is weak, it would be better to have labels.
- Not having GOs without a Transform can cause performance penalties when we want organization.
- Handling OnCollider… and OnTrigger… is quite weird from a design point of view. How do they know I want only 1 collider per object? Instead, those should be events of the Colliders components themselves that we subscribe to.
Misplaced Critiques Of The Current Component System:
- Magic methods are not necessarily bad. They are a convention. Forcing an interface would be another convention that just forces an implementation on top of that.
- Having different Start() and Awake() is not weird.
- “OOP code is hard to maintain” - No it isn’t. It’s so easy to reason about it’s ridiculous. The fact most people write crappy code is not the point. They will write crappy code in any system, since they now have to worry about more things. There is a whole body of literature about how to write OOP code properly, and it’s really not hard.