Reputation: 43
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);
}
}
}
Upvotes: 4
Views: 2055
Reputation: 21755
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.
Upvotes: 0
Reputation: 1485
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);
Upvotes: 1
Reputation: 1500585
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...)
Upvotes: 6
Reputation: 70142
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.
Upvotes: 1
Reputation: 29213
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.
Upvotes: 1