Code Quality Chronicles Vol 2 - What not to do

// [1]
[SerializeField]
private MonoBehaviour[] m_MonoBehaviours = new MonoBehaviour[0];

// [2]
private void EnableContent(bool enabled) { ... }

// [3]
if (m_MonoBehaviours.Length > 0)
{
    foreach (var monoBehaviour in m_MonoBehaviours)
    {
        monoBehaviour.enabled = enabled;
    }
}

[1] - They demonstrate the practice of serializing private fields. This convention has started to become a matter of style, but it clearly violates encapsulation. As per MSDN:

Private members are accessible only within the body of the class or the struct in which they are declared.

However, serialized private fields can be accessed through the serialization engine right from the Inspector, during runtime. The purpose of Encapsulation was to prevent unexpected access from the outside, and to hide the implementation from the outside. In this case private does not mean either: no implementation hiding and partial protection from outside access. During the compiled game this access cannot be violated, but in my opinion this doesn't change the point. What this does is prevent the field from being accessed through code at any time (the only available access is through the Inspector). This guarding is unnecessary - if the variable can be accessed at any time anyway, why guard it against code access? The reason people do this is it gives the a feeling they are doing something better than expected with more verbosity, so it's "a good practice". This colleague-paranoid, programmer-folklore-oriented attitude has to go. I would normally let this one slide, but it is coupled with other bad code... Also, it will throw errors in the console, which are obvious: "You are trying to create a MonoBehaviour using the 'new' keyword. This is not allowed."

[2] - Can you guess what this method does just by signature? It seems it enables something, but what does the argument do? It takes the argument whether something was enabled? Would you guess this actually disables content sometimes? Knowing there is something called "m_Content", would you guess this method enables or disables m_Content and optionally children of the current gameObject, and does [3]?

[3] - So now we have the star of the show. Guarding foreach with an if that checks the size first. I can only assume someone wanted to guard against the null, but didn't know how. Accessing the Length property of an array will throw an exception if the array was null, same as would foreach. Even if assignment from [1] was allowed from the Engine's point of view, Creating a 0-sized array ensures there will be garbage, arrays have to be re-assigned so any value that is assigned to m_MonoBehaviours will ensure the previous 0-sized array (which is still an object) becomes garbage at that point. Someone clearly didn't understand the difference between lists and arrays, and tried to new-up an instance like you would do with a list (you can add to lists, and they will maintain an array internally). 

If you're finding this article helpful, consider our asset Dialogical on the Unity Asset store for your game dialogues.