2
\$\begingroup\$

I'm just going to ask about this 4 Level Deep For Loop of Mine though i put it inside of the StartCoroutine();

Here is my code : please don't judge me yet

void Update()
{
    if (tzPlayInfo.Instance.BLOCK_GAME_FLOW == false)
    {
        CameraScriptsAir_1 = this.transform.GetChild(0).GetChild(1).GetChild(0).GetChild(2).GetChild(0).gameObject;

        CameraScripstAir_2 = this.transform.GetChild(0).GetChild(1).GetChild(1).GetChild(2).gameObject;

        //get the Camera with Bloom Script
        for (int i = 0; i < maxCameraScriptTV; i++)
        {
            CameraScriptsTV[i] = this.transform.GetChild(0).GetChild(1).GetChild(i + 5).GetChild(0).GetChild(0).gameObject;
        }

        //get the Camera With DepthOfField Script and Bloom Script
        TwoScript[0] = this.transform.GetChild(0).GetChild(1).GetChild(8).GetChild(0).GetChild(1).gameObject;
        TwoScript[1] = this.transform.GetChild(0).GetChild(1).GetChild(38).GetChild(0).GetChild(0).gameObject;

        //9 - 11
        //get only the depthoffield.cs
        for (int j = 0; j < maxTests1; j++)
        {
            Tests1[j] = this.transform.GetChild(0).GetChild(1).GetChild(j + 9).GetChild(0).GetChild(0).gameObject;
        }

        //get all the Test with the camera Bloom script
        for (int k = 0; k < maxTests2; k++)
        {
            Tests2[k] = this.transform.GetChild(0).GetChild(1).GetChild(k + 12).GetChild(0).GetChild(0).gameObject;
        }

        //get the fog GameObject
        FogEffect = this.gameObject.transform.GetChild(0).GetChild(2).GetChild(0).GetChild(2).gameObject;

        if (!foundAllComponents)
        {
            StartCoroutine(CountObject());
        } else
        {
            StopCoroutine(CountObject());
        }
    }


    if (foundAllComponents)
    {
        if (OptionPopup.isCheckedCamera == true)
        {
            CameraScriptsAir_1.GetComponent<DepthOfFieldDeprecated>().enabled = false;
            CameraScriptsAir_1.GetComponent<Bloom>().enabled = false;
            CameraScripstAir_2.GetComponent<DepthOfFieldDeprecated>().enabled = false;
            CameraScripstAir_2.GetComponent<Bloom>().enabled = false;

            foreach (GameObject camScriptTV in CameraScriptsTV)
            {
                camScriptTV.GetComponent<Bloom>().enabled = false;
            }

            foreach (GameObject two in TwoScript)
            {
                two.GetComponent<DepthOfFieldDeprecated>().enabled = false;
                two.GetComponent<Bloom>().enabled = false;
            }

            foreach (GameObject test in Tests1)
            {
                test.GetComponent<DepthOfFieldDeprecated>().enabled = false;
            }

            foreach (GameObject test in Tests2)
            {
                test.GetComponent<Bloom>().enabled = false;
            }
        }
        else
        {
            CameraScriptsAir_1.GetComponent<DepthOfFieldDeprecated>().enabled = true;
            CameraScriptsAir_1.GetComponent<Bloom>().enabled = true;
            CameraScripstAir_2.GetComponent<DepthOfFieldDeprecated>().enabled = true;
            CameraScripstAir_2.GetComponent<Bloom>().enabled = true;

            foreach (GameObject camScriptTV in CameraScriptsTV)
            {
                camScriptTV.GetComponent<Bloom>().enabled = true;
            }

            foreach (GameObject two in TwoScript)
            {
                two.GetComponent<DepthOfFieldDeprecated>().enabled = true;
                two.GetComponent<Bloom>().enabled = true;
            }

            foreach (GameObject test in Tests1)
            {
                test.GetComponent<DepthOfFieldDeprecated>().enabled = true;
            }

            foreach (GameObject test in Tests2)
            {
                test.GetComponent<Bloom>().enabled = true;
            }
        }

        if (OptionPopup.isCheckedFog == true)
        {
            FogEffect.SetActive(false);
        }
        else
        {
            FogEffect.SetActive(true);
        }
    } else
    {
        Debug.Log("Wait for the component to be found!!");
    }

}

IEnumerator CountObject()
{
    //count the object inside the camera objects
    for (int i = 0; i < CameraScriptsTV.Length; i++)
    {
        //count the object inside the twoscript objects
        for (int j = 0; j < TwoScript.Length; j++)
        {
            //count for tests1 objects
            for (int k = 0; k < Tests1.Length; k++)
            {
                //count for tests2 objects
                for (int l = 0; l < Tests2.Length; l++)
                {
                    //check all the objects if they're not null
                    if (CameraScriptsAir_1 && CameraScripstAir_2 && FogEffect != null && CameraScriptsTV[i] != null && TwoScript[j] != null && Tests1[k] != null && Tests2[l] != null)
                    {
                        Debug.Log("GameObject is not null");
                        foundAllComponents = true;
                    }
                    else
                    {
                        Debug.Log("Lets wait for the game object to be attached");
                    }
                }
            }
        }
    }
    yield return null;
}

On the last part of my code

IEnumerator CountObject()
{
    //count the object inside the camera objects
    for (int i = 0; i < CameraScriptsTV.Length; i++)
    {
        //count the object inside the twoscript objects
        for (int j = 0; j < TwoScript.Length; j++)
        {
            //count for tests1 objects
            for (int k = 0; k < Tests1.Length; k++)
            {
                //count for tests2 objects
                for (int l = 0; l < Tests2.Length; l++)
                {
                    //check all the objects if they're not null
                    if (CameraScriptsAir_1 && CameraScripstAir_2 && FogEffect != null && CameraScriptsTV[i] != null && TwoScript[j] != null && Tests1[k] != null && Tests2[l] != null)
                    {
                        Debug.Log("GameObject is not null");
                        foundAllComponents = true;
                    }
                    else
                    {
                        Debug.Log("Lets wait for the game object to be attached");
                    }
                }
            }
        }
    }
    yield return null;
}

This one is just for counting of my object so that i can put it inside an if else statement . I could not compare an Array of GameObject to null if i'm not going to get its Length right ? So is my way of doing it is safe or not? and did i use my coroutine right cause i'm a bit conscious of it. I'll accept any discrimination on my code.

\$\endgroup\$
  • \$\begingroup\$ Just about your last snippet: are you just testing CameraScriptsTV.All(NotNull) && TwoScripts.All(NotNull) && ...? You can write it like that (where static bool NotNull(object obj) => obj != null;). Also you're returning an IEnumerator but yeld return generates an IEnumerable<T> and...there isn't any enumeration there. What for? \$\endgroup\$ – Adriano Repetti Jan 16 '18 at 10:42
1
\$\begingroup\$

Your code is going to log an awful lot of "GameObject is not null" items, for very few non-null game objects.

Basically, you've made an O(n * m * p * q) algorithm, where all you needed was (at most) an O(n + m + p + q) algorithm. This would achieve the same effect - beyond the fact that you'll get fewer logs of the same non-null combination over and over again.

This code assumes that the library requires an enumerator.

IEnumerator CountObject()
{
    if (CameraScriptsAir_1 &&
        CameraScripstAir_2 &&
        FogEffect != null &&
        CameraScriptsTV.Any(e => e != null) &&
        TwoScript.Any(e => e != null) &&
        Tests1.Any(e => e != null) &&
        Tests2.Any(e => e != null))
    {
        foundAllComponents = true;
        Debug.Log("GameObject is not null");
    }
    else
    {
        Debug.Log("Lets wait for the game object to be attached");
    }
    yield break;
}
\$\endgroup\$
3
\$\begingroup\$

Some very general comments:

I would suggest that the code needs to be more readable, because for someone who is not familiar with it - it’s difficult to discern what is going on.

  • You can immediately improve the code just by giving better names for variables/methods. What is tzPlayInfo? What is its instance?
  • Why are we getting the children of children of children: this.transform.GetChild(0).GetChild(1).GetChild(1).GetChild(2).gameObject; - surely there’s a better way? What is gameObject - can you think of a more descriptive name. Also this looks like it's violating the law of demeter (please google it if you're not familiar with it.)
  • There are way too many if statements for it to be understandable - so perhaps rethink the algorithm.
  • I recommend breaking up the code into many separate methods - i.e. a good rule of thumb, as an exercise, would be to make each method no more than 10 lines long. This will force you to break things up. Right now it's a wall of code/text.
  • Typically folks would return an Enumerable<T> rather than an Enumerator – is there any reason why an enumerator is returned? - Avoid deep nesting. It could take a very long time as N increases.
\$\endgroup\$
1
\$\begingroup\$
  • Why do you need to return an enumerator? It's only useful if you yield return items to make a list. Here you just return null which is the same as returning true or false.

  • Why do you need the nested loop? What are you trying to do? Right now when each of the lists first item is not null, you already set the variable allFound to true but you keep on looping. So if the next item is null, you still set it to allFound. Seems strange.

  • Naming. GetChild().GetChild(). Much easier to read as GetTest(). It's also known as a "train wreck", all the nested GetChilds. One solution is to have a method in the top class return all the data you need.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.