-1

I'm trying to generate 5 random numbers between 1-10 which do not have duplicate values. Therefore, I've created a recursive method which should check to see if the value created for whichever position in the array has been used already. If it has, then it will create a new random number and check again.

Here's my code:

    static Random randomObject = new Random();

    static void Main(string[] args)
    {

    long[] randomArr = new long[5];


    for (int i = 0; i < randomArr.Length; i++ )
    {
        if (randomArr[i] == randomArr[0])
        {
            randomArr[i] = randomObject.Next(1, 11);
        }
        else 
        {
            long check = randomObject.Next(1, 11);
            randomArr[i] = CheckIfUnique(check, randomArr);
        }
    }

    Console.WriteLine("\nPress the [enter] key to continue...");
    Console.ReadLine();

}

static long CheckIfUnique(long a, long[] b) 
{
    for (int i = 0; i <= b.GetUpperBound(0); i++) 
    {
        if (a == b[i])
        {
           a = randomObject.Next(1, 11);
           CheckIfUnique(a, b);
        }
    }
    return a;
}

But I'm still getting duplicate values. Does anyone know if there is an error in my logic, or if the compiler will crap out after so many recursive steps?

  • 6
    In CheckIfUnique, you don't return CheckIfUnique(a, b), you just call it. Is that what you want? – ergonaut Nov 06 '15 at 16:45
  • 5
    Are you using recursion because you *have* to (as part of a test, perhaps), or because you think it's necessary? It's really not an appropriate solution to this problem. – Gary McGill Nov 06 '15 at 16:46
  • 3
    If you are after values between 1 and 10, why are you using long? – Karl Gjertsen Nov 06 '15 at 16:52
  • 2
    If you want a better algorithm, consider using a [Fisher-Yates shuffle](https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle). – Dan Bryant Nov 06 '15 at 16:58
  • Use a `HashSet` and keep adding to it until count = 5 – DGibbs Nov 06 '15 at 16:59
  • Since "recursive is not requirement - [duplicate](http://stackoverflow.com/questions/1287567/is-using-random-and-orderby-a-good-shuffle-algorithm) shows standard approach for generating "unique random numbers" (which is actually shuffle, because random numbers can't be unique), as well provides implementation that lat one to pick only part of the shuffled sequence (like 5 out of 10 in this post). – Alexei Levenkov Nov 06 '15 at 17:40
  • @AlexeiLevenkov Technically, my question as stated above was "Does anyone know if there is an error in my logic, or if the compiler will crap out after so many recursive steps?" Someone provided an answer to my question. And the SHUFFLE would not be an answer to this question, since this is a small snippet of a larger project that I'm working on. – Amanda Gray Nov 06 '15 at 17:59
  • @AmandaGray and you've accepted solution that has no traces of recursion with comment "This is exactly what I was looking for." I'm not really sure what make out of that comment. If you think that O(n) solution provided in linked answer (compared to accepted O(n^2)) does not work for you - please clarify what is actually required and update to post. – Alexei Levenkov Nov 06 '15 at 18:04

4 Answers4

3

NOTE - THIS IS NOT A RECURSIVE ANSWER


You can make that much easier really:

static Random randomObject = new Random();

static void Main(string[] args)
{
    long[] randomArr = new long[5];

    for (int i = 0; i < randomArr.Length;)
    {
        long t = randomObject.Next(1, 11);
        if(CheckIfUnique(t, randomArr, i))
        {
              randomArr[i++] = t;
        }
    }
}

static bool CheckIfUnique(long a, long[] b, int length)
{
    for (int i = 0; i < length; i++) 
    {
        if (a == b[i])
        {
           return false
        }
    }
    return true;
}

This way, you're looping till you have enough random values, but you're only advancing the counter once a new unique value has been generated.

Amit
  • 45,440
  • 9
  • 78
  • 110
  • 1
    `CheckIfUnique` could probably be reduced almost completely to `return !b.Contains(a);` – Ron Beyer Nov 06 '15 at 16:53
  • @RonBeyer - of course. most of this code could be refactored to something much simpler and nicer, but I tried to stay as close to the original as makes sense. – Amit Nov 06 '15 at 16:54
  • @Amit, thank you. This is exactly what I was looking for. Makes sense without it having to be recursive. – Amanda Gray Nov 06 '15 at 17:10
  • Note that this is answer should not be used outside of homework assignments for `for` loop due to linear search for already generated items. – Alexei Levenkov Nov 06 '15 at 18:11
2

your approach is really heavy weight. As a variant you can see this approach. However, if you want I can try to correct your algorithm. The order of numbers are original which is created by Random class:

HashSet<int> origNumbers = new HashSet<int>();
Random rnd=new Random();            
do
{
   int k = rnd.Next(10);
   origNumbers.Add(k);
}
while (origNumbers.Count <5);

Update(thanks to @AlexeiLevenkov):

If order of added elements is important, it is preferable to use collection SortedSet<T>. The order of numbers created by Random class are sorted in ascending order, it is not the order created by Random class:

SortedSet<int> origNumbers = new SortedSet<int>();
Random rnd=new Random();            
do
{
   int k = rnd.Next(10);
   origNumbers.Add(k);
}
while (origNumbers.Count <5);
StepUp
  • 36,391
  • 15
  • 88
  • 148
  • 1
    Since order of elements in `HashSet` is not defined you should not be storing list of items where order is important (i.e. compliant `HashSet` implementation may even return items as ordered). http://stackoverflow.com/questions/657263/does-hashset-preserve-insertion-order – Alexei Levenkov Nov 06 '15 at 18:15
  • @AlexeiLevenkov Have I caught your thought? Please, see my updated answer. – StepUp Nov 06 '15 at 20:16
  • No. What do you expect as result? Updated code collects sorted list of 5 random number with possible repetition... You may want to clearly restate question in your answer like "this code will provide ....." to clarify what exactly you are answering. Original sample was reasonable (definitely faster than currently accepted one), and may be even what OP needed - but it has undefined behavior concerning order of items - if you want to keep order use list (or array, linked list). – Alexei Levenkov Nov 06 '15 at 20:46
  • i am sorry I cannot understand your thoughts:). What order do you mean? Preserved order of created numbers by Random class or collection should have ascending order of items? – StepUp Nov 06 '15 at 20:55
  • 1
    Whatever order you think is useful - problem with `HashSet` is that it does not define any order - so it may not be randomly distributed numbers as original sequence generated by `Random`. If you clarify what order of items code provide (or specify "results in just a set of numbers randomly picked from range") that would be enough to be reasonable answer. I personally expect numbers to be in order generated by `Random` (at least OP's code tried that). – Alexei Levenkov Nov 06 '15 at 21:27
  • @AlexeiLevenkov please, check whether I catch your thoughts:) – StepUp Nov 06 '15 at 21:54
  • @AlexeiLevenkov is it really that HashSet is organized like LinkedList, however algorithm makes HashSetto be HashSet?:) – StepUp Nov 06 '15 at 22:00
1

Here is a very simple solution:

var random = new System.Random(Guid.NewGuid().GetHashCode());  
var values = Enumerable.Range(0, 10).OrderBy(x => random.Next()).Take(5).ToArray();
Joseph M. Shunia
  • 310
  • 2
  • 10
  • Does this produce an unbiased permutation? The 'comparison' function is not stable (for any given A and B it yields a random ordering), so you might expect larger numbers to be more likely to wind up toward the middle of the set, since they are added later. – Dan Bryant Nov 06 '15 at 17:06
  • @DanBryant This *is* actually a biased result, but that's because `OrderBy` *is* a state sort, so any time the random number for different entries happen to collide the earlier entry will always be first, so items that are earlier in the original results are likely to stay earlier in the final results, rather than being evenly distributed. – Servy Nov 06 '15 at 17:28
  • @Servy, I think it's a bit trickier than that. The particular Quicksort used is stable for a 'well-behaved' comparison function, but I'm not sure how it behaves when comparison of two elements always returns a new random value, regardless of whether the two elements were compared previously. I would expect at least a small bias (due to the reason you mentioned), but I suspect it's actually worse than that. It would be clearer if OrderBy used insertion sort, since that would more obviously bias later elements toward the front of the series. – Dan Bryant Nov 06 '15 at 17:33
  • 3
    @DanBryant: I don't like this solution, but I am also not following your line of thought here. Indeed, quicksort requires a consistent total ordering, and providing random results for a comparison can cause bad things to happen. But that's not what is happening here. Here we are generating a random sequence of *keys* and then sorting on those keys. The bizarre thing about this code is the use of a new guid's hash code as the seed. Why should the output of one pseudo-random-number generator be a good seed for another? – Eric Lippert Nov 06 '15 at 17:38
  • 1
    @Eric Lippert, You're right, I was confusing OrderBy with a sort using a comparison function. As long as the random keys are only computed once for each element, then cached, my arguments are invalid. – Dan Bryant Nov 06 '15 at 17:42
  • 1
    Note that this is discussed to death on SO - i.e. http://stackoverflow.com/questions/1287567/is-using-random-and-orderby-a-good-shuffle-algorithm (I've marked this post as duplicate as it already provides good solution including exact OP's case). – Alexei Levenkov Nov 06 '15 at 17:43
  • @DanBryant Yeah, it shouldn't be biased. It may not be the most efficient way to do things (with regard to the shuffling), but since we are only sorting a list of 10 items, the difference between O(n) and O(n log n) is negligible. – Joseph M. Shunia Nov 06 '15 at 19:01
0

It's not recursive, but unless it is homework, recursive would not work really well for your case

In this case, with the following assumptions:

1) No duplicates

2) Relatively small range of possible values (a few hundreds at most?)

I would populate a small initial dataset with all your possible values, and then select from those:

    static Random randomObject = new Random();
    static void Main(string[] args)
    {
        int MAX_NUMBER = 10;
        int MIN_NUMBER = 1;
        int NUMBER_OF_RESULTS_REQUIRED = 5;

        long[] randomArr = new long[NUMBER_OF_RESULTS_REQUIRED];
        //Use a pool of all possible values
        List<long> dataSet = new List<long>();


        for (int i = MIN_NUMBER; i <= MAX_NUMBER; ++i)
        {
            dataSet.Add(i);
        }

        for (int i = 0; i < NUMBER_OF_RESULTS_REQUIRED; ++i)
        {
            int t = randomObject.Next(0, dataSet.Count);
            randomArr[i] = dataSet[t];
            dataSet.RemoveAt(t);
        }
        for (int i = 0; i < randomArr.Length; ++i)
        {
            Console.WriteLine(randomArr[i]);
        }
        Console.ReadKey();
    }
Gerasimos R
  • 2,056
  • 1
  • 12
  • 22