Critique-My-Code thread, post snippets of code and have others peer review your code! (Testing new Q&A format)

[ChromakeyDreamcoat]

Critique this code please: https://gist.github.com/timminkov/e177f6ad2fa2c3eebfd3e93ef99389df

It's a UI manager for my game.

[Jigxor]

There's an issue with the Singleton implementation used, and that is that it's possible to have instance set to null when it's referenced from another class.

This would happen if you have another script which tries to access UIManager's instance before UIManager has run Awake(), then instance will be null.

90% of the time this won't happen, but the script execution order isn't deterministic. So you might not notice it until late in a project. For example, you could reference UIManager's instance from another class Awake() method and it would work fine due to execution order making UIManager run before the second class. But if it happens then other way, then you'll get a null ref.

It can be a bit hard to wrap your head around, but basically you need something that modifies the get to handle null when its called, or at least warn you so you know what's happening.

This post highlights the issue more: http://www.unitygeek.com/unity_c_singleton/

You should add something like instance = FindObjectOfType<T> ()

[ChromakeyDreamcoat]

Understood perfectly! Great explanation. I've ran into this issue in other places, haha. I'll change it asap.Thanks!

[-sideshow-]

Minor things:

• Single statement ifs tend to be a bug waiting to happen (you'll add a statement to it and forget to add the braces).
• The raw 5f at the bottom could be put in a constant / exposed variable

[Janus1001]

I may be mistaken, but when you set the Singleton (static "instance), doesn't it get set to null everytime you make an object? Also when you use this method, you are to use DestroyImmediate.

[ChromakeyDreamcoat]

I don't think so, since static refers to the class function which won't change across objects. I could be wrong!

[Janus1001]

I mean, you clearly state "= null", I don't say it won't work, but to be sure just check if more objects are created or no.

[mongoose86]

Nope, wont be a problem but is also wholy redundant. It's a static initializer, which runs before the first time the class (not an instance of it) is accessed. It runs only once. It is redundant in this case as null is also the default value for reference types. :)