Is Having a foreach loop inside a foreach loop a bad idea

Unity Engine
1

Hey, so i haev this game that uses Scriptable Objects to store the stats of each character and one of those being loyalty.

For one of the quests i want to make it so you have to have two characters that have the same loyalty to complete it. To do this i was thinking of having some code liek this but was wondering if having a loop within a loop like this is dangerous and could lead to crashing etc

foreach(CharacterSO CharOne in Characters)
{
    foreach(CharacterSO CharTwo in Characters)
    {
        if(CharOne.Loy > 0 && CharOne.Loy == CharTwo.Loy)
        {
         //Comp Quest Stuff
        }
     }
}
2

It’s not dangerous per se but there are likely optimisations to be made. But like most people on this forum will tell you, don’t optimise too early, wait until it becomes a problem.

1 Like
3

There’s no danger of crashing. Why would it crash? There is a danger of you not checking if CharOne != CharTwo, so that’s a bug :stuck_out_tongue:

2 Likes
4

I don’t understand the code, you are (also) comparing each object to itself multiple times? A nested for loop generally has the inner loop based on each object in the outer loop. You basically have two outer loops. Can you describe what your intention is? It seems you just want to find all characters with the same Loy value? You are close, but you are also doing you comp quest stuff for each object when compared to itself, I’m guessing not what you want. Your quest logic will trigger for every CharOne with Loy greater than 0 at least once (when comparing to itself). So basically the same bug that Baste points out.

5

Nested loops are fine, and won’t lead to crashing per se - unless you make a mistake. However nesting loops can be very costly in terms of performance depending on how many items they must loop over.

There’s something called big-O notation, that is used to express the cost of running code. In this case, your code has quadratic cost, or O(NN) in big-O notation. This means that for N characters, your code does NN operations. So if you have 10 characters, the code inside the inner for loop will run 100 times. If you have 100 characters, it will run 10000 times, etc: The amount of work your code does grows much faster than the number of characters.

For comparison, a single for loop usually has linear cost: O(N). If there’s N elements, the for loop runs N times.

Your nested loops are fine if you have few (say, 10) characters. But if you plan on having hundreds or thousands of characters then it’s something that you should avoid if you want good performance.

6

In case Loy is a float, your code has to look something like this:

        float allowedVariance = 0.001f;

        for (int i = 0; i < Characters.Count - 1; i++)
        {
            for (int j = i + 1; j < Characters.Count; j++)
            {
                float distance = Mathf.Abs(Characters[i].Loy - Characters[j].Loy);
                if (distance < allowedVariance)
                {
                    //Comp Quest Stuff
                }
            }
        }

In case it is an int and the logic you posted was required to you, your code has to look something like this:

        for (int i = 0; i < Characters.Count - 1; i++)
        {
            for (int j = i + 1; j < Characters.Count; j++)
            {
                if (Characters[i].Loy > 0 && Characters[i].Loy == Characters[j].Loy)
                {
                    //Comp Quest Stuff
                }
            }
        }