0

(this is a library)
The function GetUniqueInt is being called with (5, 5) as the variables.
Currently the code will stall unity to a complete halt, or crash my PC with a memory overflow error.
Does anyone have any ideas as to how I could prevent it from crashing or what is making it crash?

using UnityEngine;

namespace MajorSolution
{
    public static class MajorMath
    {
        public static int[] GetUniqueInt(int intCount, int intLength)
        {
            int[] returnValue = new int[intCount];
            int[] temp = new int[intLength];
            for (int a = 0; a < intCount; a++)
            {
                string create = new string("create".ToCharArray());
                switch (create)
                {
                    case "create":
                        returnValue[a] = GetRandomInt(intCount);
                        goto case "check";
                    case "check":
                        bool alreadyTaken = false;
                        for (int c = 0; c < returnValue.Length - 1; c++)
                        {
                            if (returnValue[a] == returnValue[c])
                            {
                                // Already Taken!
                                alreadyTaken = true;
                            }
                        }
                        if (!alreadyTaken)
                        {
                            break;
                        }
                        else
                        {
                            goto case "create";
                        }
                    }
                }
            Debug.Log(returnValue);
            return returnValue;
        }

        public static int GetRandomInt(int intCount)
        {
            int[] storage = new int[intCount];
            int returnValue = 0;
            for (int i = 0; i < intCount; i++)
            {
                storage[i] = (Mathf.FloorToInt(Random.Range(0, 9)) * (int)Mathf.Pow(10,i));
                returnValue += storage[i];
            }
            return returnValue;
        }
    }
}
abatishchev
  • 98,240
  • 88
  • 296
  • 433
  • It is unclear what method is expected to do, but I strongly suspect you are looking for https://stackoverflow.com/questions/273313/randomize-a-listt. – Alexei Levenkov Apr 27 '18 at 23:04
  • 1
    Seems like infinite loop with the `goto` keyword. Hard to suggest a fix because you didn't explain what that function is supposed to be doing but I suggest staying away with the `goto` keyword. – Programmer Apr 27 '18 at 23:05
  • This line `returnValue[a] == returnValue[c]` will always evaluate to true at some point in your loop since you have just set `returnValue[a]` before entering this loop. – pstrjds Apr 27 '18 at 23:06
  • As a side note - you do not need to write code like this `string create = new string("create".ToCharArray());` to initialize a string with a constant string. You can just write `string create = "create";` – pstrjds Apr 27 '18 at 23:26
  • @AlexeiLevenkov It was a method to generate an array of ints that are all unique from one another. – Shane Powell Apr 27 '18 at 23:44
  • @Programmer the goto keyword was not the problem, but rather an issue with my if-then statement. As pstrjds stated the solution below your comment the only error causing the infinite loop was the if-then statement. Although in his answer he did provide much cleaner code which produced like results. – Shane Powell Apr 27 '18 at 23:44
  • 2
    `goto` is not directly the problem. The problem is how you used the `goto` keyword. It will keep returning to where you tell it to go to resulting to infinite loop. It can easily introduce spaghetti code. If you can write a code without it, do that. pstrjds's answer managed to do that without `goto`. – Programmer Apr 27 '18 at 23:55

1 Answers1

3

Edit I just realized I did not exactly answer the question of why it is bringing the PC to a halt because you have an infinite loop in the code.

The problem is occurring in the following lines of code, notice what is happening.

case "create":
    returnValue[a] = GetRandomInt(intCount);
    goto case "check";

In the above block of code you are generating a number and putting it in the returnValue array. Now you jump into your "check" block

case "check":
    bool alreadyTaken = false;
    for (int c = 0; c < returnValue.Length - 1; c++)
    {
        if (returnValue[a] == returnValue[c])
        {
            // Already Taken!
            alreadyTaken = true;
        }
    }

In this block of code you are looping over the entire returnValue array including the value you just inserted in it. Basically you are looping over the array asking if a value that you just put in the array is in the array.

Without knowing exactly what you are trying to do with these methods, I will just make a simple fix suggestion with some minor cleanup

public static int[] GetUniqueInt(int count, int length)
{
    var returnValue = new int[count];
    var values = new HashSet<int>(); // Used to track what numbers we have generated
    for (int i = 0; i < count; ++i)
    {
        // Generate the number and check to be sure we haven't seen it yet
        var number = GetRandomInt(length);
        while(values.Contains(number)) // This checks if the number we just generated exists in the HashSet of seen numbers
        {
            // We get here if the HashSet contains the number. If we have
            // seen the number then we need to generate a different one
            number = GetRandomInt(length);
        }

        // When we reach this point, it means that we have generated a new unique number
        // Add the number to the return array and also add it to the list of seen numbers             
        returnValue[a] = number;
        values.Add(number); // Adds the number to the HashSet
    }
    Debug.Log(returnValue);
    return returnValue;
}

I did end up removing the using of intLength but from your posted code it was only used to declare a temp array which itself was never used. Based on that, I just removed it entirely.

Based on your comment I updated the fix to use intLength. I made one other minor change. I removed int from the variable names of count and length. Hungarian notation is a lot less common in C# code. Personally, I feel like the code is cleaner and easier to read without the Hungarian notation. The key is to use good variable names that express the intent or make it easier to follow. In this case count is the count (read total number) of numbers you want returned and length is the length of the number. You could consider maybe even renaming that to numberOfDigits to make it clearer that the idea is you are going to create a random number with that number of digits in it.

pstrjds
  • 16,840
  • 6
  • 52
  • 61
  • I can see where the error is coming from now. What would be a nice way to check if the value already exists in the array? The only reason I used.Length - 1 is because I thought it would not include the current one, but I realized that it's not creating new ones. So I guess either how do you increase the size of an array that already exists or how do you check only for values that are not zero or the one you just created? – Shane Powell Apr 27 '18 at 23:16
  • I have an idea for how I might be able to fix this now, but if it fails I am interested in a possible solution. – Shane Powell Apr 27 '18 at 23:18
  • The changes you have made are beyond my knowledge, so I will have to research a few things after I get this working. A few things about the changes you made: the use of the variables u and a should be changed to i. Also intLength was to determine the length of the array. – Shane Powell Apr 27 '18 at 23:27
  • @ShanePowell - I made a typo, that should just be `i` not `u` and `i`. It is fairly standard practice to use `i,j,k` as your for loop variables, especially when they are used as indexers into an array. Everyone understands them. You use `i` in the outer loop and if you have nested for loops then you move to `j` and then `k`. I removed `intLength` in the fix I suggested because you were not using it in your code. You only used it to declare the length of the `temp` array, but the `temp` array was never used. Once that was gone there was no more use of the `intLength` variable. – pstrjds Apr 27 '18 at 23:31
  • https://hastebin.com/alutisetuq.cs Sorry for the external link, but there is the final thing that I had meant. Thank you for the working solution~ – Shane Powell Apr 27 '18 at 23:38
  • @ShanePowell - No problem. I thought that was the intention of your code, but since the code you posted did not show that I stuck with what was posted. If you have questions about something in the answer I posted please feel free to ask. – pstrjds Apr 27 '18 at 23:43
  • The only thing I am curious about is the HashSet and the use of While, but beyond that, I should have known better. I will look into these two things more so I can further my knowledge. Thank you for the help :D – Shane Powell Apr 27 '18 at 23:47
  • @ShanePowell - A [HashSet](https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.hashset-1?view=netframework-4.7.1) is the .Net version of a `set` data structure. In regards to `while`, it is just another type of looping construct. – pstrjds Apr 27 '18 at 23:55