0
public static List<int> GetRandom()
{
    Random rnd = new Random();
    List<int> list = new List<int>();

    while (list.Count <= 26)
    {
        int randomNumber = rnd.Next(1, 26);

        if (!list.Contains(randomNumber))
        {
            list.Add(randomNumber);
        }
    }

    return list;
}

This is a code where I have tried to get a random list of integer(from 1 to 26) but this doesn't return me the desired result. Here I want a random int array without any repeat.

Johnny
  • 8,939
  • 2
  • 28
  • 33
Chris Hadfield
  • 494
  • 1
  • 7
  • 34
  • I think you want this one:https://stackoverflow.com/questions/30014901/generating-random-numbers-without-repeating-c – hassan.ef Apr 23 '19 at 18:43
  • 4
    Your `rnd` should be created once for the run of the application, not created on each call to the method. – crashmstr Apr 23 '19 at 18:44
  • You don't want them to be random only. You want them unique too. – Theodor Zoulias Apr 23 '19 at 19:20
  • @TheodorZoulias - Yes sir and i already got the answer – Chris Hadfield Apr 23 '19 at 19:25
  • 1
    @Nishan you have the answer but your method is a bit inefficient. Because every iteration you will have a higer probability to generate an already existing number, so you probability will go from 1/n, 2/n...n-1/n... You will waste some time looping untill you actually generate unique number... – Johnny Apr 23 '19 at 19:35

4 Answers4

5

thats because you are trying to get numbers between 1-25, so your code will never leave loop. you should call random like this

int randomNumber = rnd.Next(1, 27);
Paulo Campez
  • 702
  • 1
  • 8
  • 24
  • 2
    To expand, the first parameter to [`Next()`](https://learn.microsoft.com/en-us/dotnet/api/system.random.next?view=netframework-4.8#System_Random_Next_System_Int32_System_Int32_) is *inclusive* but the second parameter is *exclusive*. Meaning that the first parameter should be the lowest possible number in your range of desired numbers, and the second parameter should be the highest possible number **+ 1**. – Lews Therin Apr 23 '19 at 18:48
  • 1
    I _think_ this will also result in an infinite loop since Nishan is doing `while(list.Count <= 26)` – StoriKnow Apr 23 '19 at 18:52
  • Doesn't happen anything – Chris Hadfield Apr 23 '19 at 18:53
  • 1
    @Nishan as a test, you can change your `while` loop condition to `while(list.Count < 26)`, rather than `<=`. Alternatively, you could keep the while as-is and increase the second argument to the `Next` method, as outlined by `Paulo`, to `28`. Otherwise you're still in an infinite loop. – StoriKnow Apr 23 '19 at 18:54
  • 1
    @Sam You are right. It's working now. Thanks for your comment here – Chris Hadfield Apr 23 '19 at 18:56
5

Actually, you want to randomize the range of integers. You could do this using System.Linq

Random rnd = new Random();
Enumerable.Range(1, 27).OrderBy(_ => rnd.Next())

.NET Fiddle


I even measure and compare two solutions using BenchmarkDotNet, even though I was pretty sure, just as confirmation. Two scenarios have been measured, the original and one with 1000 random elements. You could see performance degradation if you increase the number of the elements(which is logical as with the increase of the number of the elements you have an even higher probability to have the collision).

BenchmarkDotNet=v0.11.5, OS=Windows 7 SP1 (6.1.7601.0)
Intel Core i7-6700 CPU 3.40GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
Frequency=3328320 Hz, Resolution=300.4519 ns, Timer=TSC
[Host]     : .NET Framework 4.6.2 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.2650.0
DefaultJob : .NET Framework 4.6.2 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.2650.0

n=26

| Method |     Mean |     Error |    StdDev | Rank |
|------- |---------:|----------:|----------:|-----:|
|   Your | 4.463 us | 0.0882 us | 0.1936 us |    2 |
|   Mine | 2.597 us | 0.0235 us | 0.0220 us |    1 |

n=1000

| Method |       Mean |       Error |      StdDev | Rank |
|------- |-----------:|------------:|------------:|-----:|
|   Your | 6,095.8 us | 119.4976 us | 122.7152 us |    2 |
|   Mine |   148.1 us |   0.6086 us |   0.5692 us |    1 |
Johnny
  • 8,939
  • 2
  • 28
  • 33
  • Can you tell me that which one is the best approach in terms of performance and why? – Chris Hadfield Apr 24 '19 at 03:07
  • 1
    @Nishan The performances are degradated because during the `Random.Next` you might have collision so you waste one loop, with my proposal you don't have collision so the leading time is `OrderBy` complexity n*logn in average. See the update... – Johnny Apr 24 '19 at 06:10
  • Got it. Thanks for your time – Chris Hadfield Apr 24 '19 at 14:35
0

As an alternative you can use MathNet.Numerics library:

PM> Install-Package MathNet.Numerics

public static List<int> GetRandom()
{
    var arr = Combinatorics.GeneratePermutation(25);
    return new List<int>(arr);
}

You may need add 1, as it generates zero-based array.

Here is relevant documentation:

Generate a random permutation, without repetition, by generating the index numbers 0 to N-1 and shuffle them randomly. Implemented using Fisher-Yates Shuffling.

koryakinp
  • 3,989
  • 6
  • 26
  • 56
0

Keeping the Random outside of the method as a static variable ensures that you'll always get a different list of numbers, even if you call the method many times in quick succession.

private static Random StaticRandom = new Random();

public static List<int> GetUniqueRandomNumbers_From_1_to_26()
{
    return Enumerable.Range(1, 26).OrderBy(_ => StaticRandom.Next()).ToList();
}

Usage example:

Console.WriteLine(String.Join(", ", GetUniqueRandomNumbers_From_1_to_26()));

Output:

26, 19, 22, 24, 16, 20, 5, 1, 8, 6, 10, 14, 13, 18, 15, 12, 25, 2, 4, 9, 21, 7, 23, 11, 3, 17
Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104