After a while random number generator gives same number

Question

I have a code that generates a user specified amount of random number from a range of also user specified numbers without repeats. The problem I have is that for a while the program works fine and then it just keeps giving the same number it seems, which breaks it.

Here's the whole code, some of it is in hungarian but if you want to try it for yourself to see what the problem really is: the program first asks you the range of numbers(first the minimum then the maximum number) and then the amount of numbers you want generated. After that you will see the your numbers generated... or not if it decides to not work. If you did get your numbers if you press "1" it will generate again with the same data you provided, pressing "2" will allow you to give different parameters and pressing "0" will quit the program.

If needed I can translate the program to help solve it.

using System;
using System.Collections.Generic;

namespace numgen
{
    class Program
    {
        static int quantity, min, max, temp;
        static bool check = false;
        static int control = 2;

        public static void Main(string[] args)
        {
            var gen = new List<int>();
            Random rnd = new Random();



            while(control > 0) {

                Console.Clear();

                if(control == 2) {
                    Console.Write("Add meg a minimum számot: ");
                    min = int.Parse(Console.ReadLine());
                    Console.Write("Add meg a maximum számot: ");
                    max = int.Parse(Console.ReadLine());
                    Console.Write("Add meg a hány számot kérsz: ");
                    quantity = int.Parse(Console.ReadLine());
                }

                gen.Clear();
                //gen.Add(rnd.Next(min, max));

                while(gen.Count < quantity) {
                    temp = rnd.Next(min, max);
                    foreach(int num in gen) {
                        if(temp == num) {
                            check = true;
                            break;
                        }
                    }
                    if(!check) {
                        gen.Add(temp);
                        //Uncomment to see number getting generated and when the program breaks
                        //Console.WriteLine(gen[gen.Count-1]);
                    }
                }

                gen.Sort();
                foreach(int num in gen){
                    Console.WriteLine(num);
                }

                Console.WriteLine("\n[2] Új adatok megadása");
                Console.WriteLine("[1] Számok újragenerálása");
                Console.WriteLine("[0] Kilépés");
                control = int.Parse(Console.ReadLine());
            }
        }
    }
}

Answer 1

A HashSet is an excellent choice for this task. Elements are added as a key--so it will refuses any value that is already in the set.

Try something like this:

    private static void TestHashSet(int min, int max, int quantity)
    {
        var gen = new HashSet<int>();
        Random rnd = new Random();

        while (gen.Count < quantity)
        { 
            var temp = rnd.Next(min, max);
            gen.Add(temp);
        }

        gen.AsEnumerable().OrderBy(s => s).ToList().ForEach(x => Console.WriteLine(x));
    }

Answer 2

Issues found in your script

1. When you set check = true;, you Never change it to false. gen.Add() never happens.
2. if someone selects minimum 2, max 3 and ask for 10, it will always have duplicate values.. Your code doesnt allow that and will run infinite loop. Calculate total numbers that can be generated between the two numbers and see if it can produce required quantity.
3. Not an issue but I would recommend using gen = new List<int>(); instead of Clear()
4. You can use Contains method to quickly check if the value is part of the list or not (instead of iterating over the entire list each time).

Updated your code and got it working like this,

    while (control > 0)
    {

        Console.Clear();

        if (control == 2)
        {
            Console.Write("Add meg a minimum számot: ");
            min = int.Parse(Console.ReadLine());
            Console.Write("Add meg a maximum számot: ");
            max = int.Parse(Console.ReadLine());
            Console.Write("Add meg a hány számot kérsz: ");
            quantity = int.Parse(Console.ReadLine());
        }
        else if (control == 1)
            gen = new List<int>();

        if (max - min < quantity)
            Console.WriteLine($"You cannot generate {quantity} between [{min} and {max}]");

        else
        {
            while (gen.Count < quantity) 
            {
                temp = rnd.Next(min, max);
                if (!gen.Contains(temp))
                    gen.Add(temp);
            }

            gen.Sort();
            gen.ForEach(x => Console.WriteLine(x));
        }

        Console.WriteLine("\n[2] Új adatok megadása");
        Console.WriteLine("[1] Számok újragenerálása");
        Console.WriteLine("[0] Kilépés");
        control = int.Parse(Console.ReadLine());
    }

Answer 3

You're not setting check back to false at any point in time.

Once you get the first collision you cease adding value to the list, so while (gen.Count < quantity) will never be true.

You might want to consider using a keyed Dictionary which will handle the indexing for you rather than some logic.

Also you're not checking if quantity is greater than the provided range. If it is you will always get collisions.

Consider a keyed dictionary, to take the logic checking away and let the Dictionary object take care of it.

var gen = new Dictionary<int, int>();

if (quantity > (max - min))
    throw new ArgumentOutOfRangeException("Quantiy must be smaller than range");

while (gen.Count < quantity)
{
    temp = rnd.Next(min, max);

    if(!gen.ContainsKey(temp))
        gen.Add(temp, temp);
}

foreach (int num in gen.Values)
{
    Console.WriteLine(num);
}

Answer 4

You are not setting check back to false, so your code breaks after the first duplicate is found.