A couple of questions about my GameManager

Hello! I had a couple of questions about my GameManager.

Question 1:
I have a list of GameObjects that consists of every GameObject in the current scene, then it iterates through a for loop to filter out every object that isn’t tagged “Brick”, like so:

public static List<GameObject> find_bricks()
{
    Scene curr_scene = SceneManager.GetActiveScene();
    List<GameObject> bricks = new List<GameObject>(curr_scene.GetRootGameObjects());
    int brick_count = bricks.Count;

    for (int j = 0; j < brick_count; j++)
    {
        if (bricks[j].tag != "Brick")
        {
            bricks.Remove(bricks[j]);
            brick_count = bricks.Count;
            j = 0;
        }
    }

    return bricks;
}

The problem:
In every scene, there is always one object that is not filtered. It remains in the list at bricks[0]. Here’s a screenshot of the scene as well as the Debug.Log of the list in the console:

Here’s another to show it’s not just one object, but multiple:

Question 2:
So I’ve been coding in Unity for about 5+ years now and before I show the entire code I wanted to ask for some feedback or anyways to improve my code’s efficiency/formatting etc. Among my circle of friends/connections I’ve always been the go to guy for programming questions, and have never had anyone better than me to mentor me or help me improve upon my skills (Most are artists which I am very far from). So that’s basically what I’m asking of this community. I’ve come here many times, but have rarely ever posted anything on this forum (Mostly because any questions I’ve had have been answered). Thank you very much for your time, and I appreciate this community for all of the help it has given me throughout the years!

Entire Code:

Game_Man(C#):

using UnityEngine;
using System.Collections;
using System.Collections.Generic;
using UnityEngine.SceneManagement;

public class Game_Man
{
    public static void change_level()
    {
        Scene curr_scene = SceneManager.GetActiveScene();
        int sceneCount = UnityEngine.SceneManagement.SceneManager.sceneCountInBuildSettings;
        string[] scenes = new string[sceneCount];
        int curr_scene_num = 0;

        for (int i = 0; i < sceneCount; ++i)
        {
            scenes *= System.IO.Path.GetFileNameWithoutExtension(UnityEngine.SceneManagement.SceneUtility.GetScenePathByBuildIndex(i));*

if (curr_scene.name == scenes*)
_
{_
curr_scene_num = i;
_
}_
_
}*_

curr_scene_num = curr_scene_num + 1;
SceneManager.LoadScene(scenes[curr_scene_num]);
}

public static List find_bricks()
{
Scene curr_scene = SceneManager.GetActiveScene();
List bricks = new List(curr_scene.GetRootGameObjects());
int brick_count = bricks.Count;

for (int j = 0; j < brick_count; j++)
{
if (bricks[j].tag != “Brick”)
{
bricks.Remove(bricks[j]);
brick_count = bricks.Count;
j = 0;
}
}
Debug.Log(bricks[0]);
Debug.Log(bricks[1]);

return bricks;
}

public static IEnumerator check_bricks(List list)
{
while(true)
{
yield return null;
if (list[1].GetComponentInChildren() == null)
{
change_level();
}
}
}
}
brick_man(C#):
using UnityEngine;

public class brick_man : MonoBehaviour
{
private void Start()
{
StartCoroutine(Game_Man.check_bricks(Game_Man.find_bricks()));
}
}

Answer 1:

I am sure that your problem doesn’t originate here. The for loop should work flawlessly.
Two optimizations come to mind, though:

Edit: As pointed out by Bonfire-Boy, i have changed the code below to work correctly.

for (int j = 0; j < brick_count; j++)
{
    if (bricks[j].tag != "Brick")
    {
        bricks.Remove(bricks[j]);
        brick_count--;
        if (brick_count == 0) { break; }
        j--;
    }
}

There is no need to query bricks.count if you know you just removed a single item.
The bigger improvement is not restarting at 0. All you have to do is continue the loop
on the current value of j, which can be done by j–.


Answer 2:

Your formatting looks fine to me. Your variable/function names make sense to me. Personally i wouldn’t use underscores, as i prefer CamelCase. Hard to judge someones code from such a small amount of code, but i would assume you are doing fine in that department =)

Your check_bricks() function makes me uncomfortable. Without even checking if the list for null you are accessing element 1. Assuming the list does have at least two elements.
I’m not a fan of coroutines at all, but i would write it like this:

public static IEnumerator check_bricks(List<GameObject> list)
{
    UnityEngine.Assertions.Assert.IsTrue(list != null, "list must not be null!");
    UnityEngine.Assertions.Assert.IsTrue(list.count > 0, 
                                         "list does not have enough elements!");

    while(true)
    {
        yield return null;
        if (list[1].GetComponentInChildren<Brick>() == null)
        {
            change_level();
        }
    }
}   

Depending on the scenario i could also end up with:

public static IEnumerator check_bricks(List<GameObject> list)
{
    while(true)
    {
        yield return null;
        if ((list != null) && 
            (list.count > 0) && 
            (list[1].GetComponentInChildren<Brick>() == null))
        {
            change_level();
        }
    }
}   

Which i would use if i need the coroutine to run despite of any errors, maybe because it does more than just check_bricks. You could also mix the two, so you get alerted by the assert and there is still 0% chance that the code will crash in a release build without the asserts.

Not sure why that coroutine uses bricks[1], but this looks pretty suspicious to me.