-2

I'm writing a trivia game player app in C#, where the user must pick 5 randomly selected trivia questions from a set of 20 available questions.

I use this code to generate an index that the app will use to fetch the questions:

    private void GenerateRandomNumbers(int RandomIndex)
    {
        Random rand = new Random();
        HashSet<int> check = new HashSet<int>();
        for (int i = 1; i < RandomIndex; i++)
        {
            int curValue = rand.Next(1, RandomIndex);
            while (check.Contains(curValue))
            {
                curValue = rand.Next(1, RandomIndex);
            }
            ChosenQuestions.Add(Questions[curValue]);

            check.Add(curValue);
        }
        check.Clear();
    }

The problem is that when the user plays the game a second time (by clicking the "PLAY AGAIN" button) and the above code is executed again, it keeps picking the same random numbers, though they are not necessarily in the same order. this causes the same trivia questions to be chosen every time. I want a fresh new set of totally random numbers every time the above code executes. How can I modify the code to accomplish this?

NOTE: I did copy this code from somewhere else here on Stack Overflow, so it's not my own original code.

user2272380
  • 103
  • 1
  • 2
  • 13
  • Of course it picks the same numbers you hav e the same range and you check if the number is already contained and if it is you generate again?! – Christo S. Christov Jan 11 '15 at 23:25
  • If you want different value define a different range. – Christo S. Christov Jan 11 '15 at 23:25
  • If you don't change the value of RandomIndex between the two calls you will get always a random number between 1 and RandomIndex. I suggest to pass also a lower limit – Steve Jan 11 '15 at 23:31
  • I'm not sure what you expect this method to do, but it will return the values from [1 to RandomIndex-1] in a random order every time it is called. – RJ Lohan Jan 11 '15 at 23:31
  • @RJLohan From what I understand his problem is that he wants to make sure that the questions previously asked never get asked again and that makes sense. Correct me if I'm wrong OP. – Christo S. Christov Jan 11 '15 at 23:33
  • Suffle your 20 items and take the first 5. http://www.dotnetperls.com/fisher-yates-shuffle – EZI Jan 11 '15 at 23:36
  • Don't use that code. Instead, shuffle an ordered list. – SLaks Jan 11 '15 at 23:36
  • Before you mark this as duplicate it was much better to wait a bit for the response of OP.I don't think he explained the problem well but marking the thread as duplicate is a bit of a rush. – Christo S. Christov Jan 11 '15 at 23:40
  • I am nominating this for re-opening. It was hastily closed, IMO. Yes the issue in the alleged duplicate is a potential issue, but it is not the source the problem the OP is seeing. – Mike Zboray Jan 11 '15 at 23:46
  • Coding Chief, you're the closest to understanding my problem. Say I have a trivia game with 50 questions. The user decides to play only a 5-question game. That means 5 questions are randomly selected from the 50. Also, "Questions" as in Questions.Length is actually a list, thus I wrote Questions.Count() for the upper limit of the range. I rewrote the code as follows: – user2272380 Jan 12 '15 at 20:23
  • You can do this by keeping the previous questions you asked and then generate from the set of questions not asked already.Then if you reach 20 questions just set all the questions to not asked again. – Christo S. Christov Jan 11 '15 at 23:31

3 Answers3

1

Assuming RandomIndex is the number of values to pick. You want this:

rand.Next(1, RandomIndex);

to be

rand.Next(0, Questions.Length);

There is an off by one error in the loop as well. Let's just fix the whole thing:

private void GenerateRandomNumbers(int RandomIndex)
{
    Random rand = new Random();
    HashSet<int> check = new HashSet<int>();
    for (int i = 0; i < RandomIndex; i++)
    {
        int curValue = rand.Next(0, Questions.Length);
        while (check.Contains(curValue))
        {
            curValue = rand.Next(0, Questions.Length);
        }
        ChosenQuestions.Add(Questions[curValue]);
        check.Add(curValue);
    }
}

Also, if you call this in a tight loop, you will get the same values picked repeatedly. This because Random chooses a seed based on the system clock which only upadates every 15 ms or so.

I think that a shuffle would be better approach rather than checking whether indexes have already been picked.

Community
  • 1
  • 1
Mike Zboray
  • 39,828
  • 3
  • 90
  • 122
-2

You could seed RNG with different seeds

Random rand = new Random(newSeed);

Seed could be taken from current time, for example

Severin Pappadeux
  • 18,636
  • 3
  • 38
  • 64
-2

Try using just one global Random object, and not creating a new Random() every time the user plays. This will ensure that isntead of initializing a new seed every time, the numbers will be a sequence created by one seed. This will make them closer to real randomness.

Ilya Kogan
  • 21,995
  • 15
  • 85
  • 141
  • While that is an issue with the code, it is not the source of the OP's problem. There is a flaw in the range to select the random values from. – Mike Zboray Jan 11 '15 at 23:45