Using Random in generating 50 random numbers in c#

Question

I want to take 50 numbers at random so that there must be no repetition in them using random method.

Below is my code so far:

private void settext()
{      
     int i;
     Queue <int> qe= new Queue<int>(50);
     Random rm= new Random();
     for (int g = 0; g < 50; g++)
     {
         i = rm.Next(1, 50);
         if (!qe.Contains(i))
         {
              qe.Enqueue(i);
         }                
     }
 }

What's wrong with it? Does it throw an exception or does it not work properly? — matthewr, Dec 25 '12 at 09:11
What exactly do you need ? 50 unique random numbers or 20 ? your title says 50 and your question text says 20 — Habib, Dec 25 '12 at 09:23

score 6 · Answer 1 · edited May 23 '17 at 12:04

Rather than looping until you find a number which you haven't used yet, I'd suggest just creating a list (or array) with the 50 possible numbers in, and shuffling it. You can then take however many of them you want.

There are plenty of shuffling questions on Stack Overflow, such as this one.

The advantage of this is that the performance is entirely predictable and linear - whereas if you take all 50 numbers out of 50, at the end you'll have to keep generating random numbers until you happen to get the last one. Not so bad for 50, but imagine if you have hundreds of thousands of numbers...

(Also note that your existing code doesn't use the number 20 anywhere, which should ring alarm bells if you're trying to generate just 20 numbers...)

Bilal Hashmi · Answer 2 · 2012-12-25T10:54:05.487

1

Only problem with your code is that if duplicates are found, you are still incrementing the loop and not getting all the values (50) in your queue. You may use while loop, and only increment the index when a non duplicate value is found.

int index=0;
int i;
Queue<int> qe = new Queue<int>(50);
Random rm = new Random();
while(index< 50)
{
    i = rm.Next(1, 51); //to get from 1 to 50
    if (!qe.Contains(i)) //to check for duplicate
    {
        qe.Enqueue(i);
        ++index;
    }

}

The above will generate 50 unique random numbers, if you want to take 20 numbers from them then:

var numbers = qe.Take(20);

edited Dec 25 '12 at 10:54

answered Dec 25 '12 at 09:13

Bilal Hashmi

1,465
13
12

@BilalHashmi I assume the downvote is because your code is an infinite loop. You can't find 50 unique integers in the range 1 to 49. Or possibly because you use an index counter when the Queue already has a count. – JasonD Dec 25 '12 at 10:10
@Habib It's an infinite loop here. Random.Max()'s upper bound is exclusive: http://msdn.microsoft.com/en-us/library/2dx6wyd4(v=vs.100).aspx – JasonD Dec 25 '12 at 10:40
@JasonID, Thanks for pointing that out. I have just read that and modified the answer accordingly. – Bilal Hashmi Dec 25 '12 at 10:55

score 1 · Answer 3 · answered Dec 25 '12 at 09:15

1

You're forcing it to find 50 distinct numbers while only allowing it to generate them out of a range of 49 possible ones, I think. Try rm.Next(50) + 1 instead.

answered Dec 25 '12 at 09:15

Theodoros Chatzigiannakis

28,773
8
68
104

score 1 · Answer 4 · answered Dec 25 '12 at 09:17

1

How about using Linq?

private static Random rand = new Random();

var twentyUniqueNumbers = RandomNumberStream().Distinct().Take(20);

IEnumerable<int> RandomNumberStream()
{
  yield return rand.Next(1,50);
}

Or better still, create a list of 50 numbers, shuffle then take 20 ...

var twentyUniqueNumbers = Enumerable.Range(0,50)
                                    .OrderBy(s => rand.Next());
                                    .Take(20);

This will give a more predictable performance.

answered Dec 25 '12 at 09:17

ColinE

68,894
15
164
232

I wouldn't recommend using `OrderBy` like that. Yes, it'll probably *work* - but it's inefficient for no benefit, and doesn't indicate what you're actually trying to do, IMO. There are much better ways of shuffling. – Jon Skeet Dec 25 '12 at 09:21
@JonSkeet it's OK in this context, but you are right ... http://stackoverflow.com/questions/1287567/is-using-random-and-orderby-a-good-shuffle-algorithm – ColinE Dec 25 '12 at 09:23

TimothyP · Answer 5 · 2012-12-25T15:56:19.940

Following your logic it might be simpler to write this:

var results = new HashSet<int>();
var random = new Random();
while (results.Count < 20)
{
    results.Add(random.Next(1, 50));
};

There is no need to check if the number has already been added to the Hashset as every number will be added only once...

However... this is just a quick fix to help you understand the problem you're trying to solve, (it is basically the class room example) but you should really take the advice provided by Jon Skeet as you have no idea how long it will actually take to execute the code above.

Another thing to note is that you are using the Queue class which is intended to be used like a FIFO buffer... that doesn't really apply to your problem.

Using Random in generating 50 random numbers in c#

5 Answers5