-2

i would like to have 6 non equal- random numbers for my listbox.

private void Button1_Click(object sender, EventArgs e)
{
    listBox1.Items.Clear();

    int[] numbers = new int[5];

    Random rnd = new Random();

    for (int i = 0; i < numbers.Length; i++)
    {
        numbers[i] = rnd.Next(1, 50);
        if (Array.IndexOf(numbers, i) == -1)
        {
            listBox1.Items.Add(numbers[i]);
        }
    }
}

i can't figure out what mistake i have made about this code.

Esra Sancak
  • 5
  • 2

1 Answers1

2

The problem is your for loop that continue to increment its indexer even if you haven't found an unique number and your array is filled immediately without checking if the number is already present. In this way you have a list that sometimes has less items (unique) than the numbers array (with duplicates).

The best approach in this case is to use, instead of a for loop, a while loop with an explict indexer that you increment when you find an unique number until it reaches the last position in the array

int[] numbers = new int[6];

Random rnd = new Random();

// First position in the array to fill
int index = 0;
while(index < number.Length)
{
    int number = rnd.Next(0, 50);
    if (!numbers.Contains(number))
    {
        // OK we don't have it add to the array and to the list
        numbers[index] = number;
        listBox1.Items.Add(numbers[index]);

        // Point to the next position to fill
        index++;
    }
}

Now there is also a different and very elegant way to do the same thing with a bit of linq:

var indexes = Enumerable.Range(1, 50).ToList();
Random rnd = new Random();
int[] numbers = indexes.OrderBy(x => rnd.Next()).Take(6).ToArray();

But then you need a loop to add the numbers array to the list and probably, as pointed out in the comment below, it could be not very efficient

Steve
  • 213,761
  • 22
  • 232
  • 286
  • 1
    `listBox1.DataSource = numbers;` or directly: `listBox1.DataSource = Enumerable.Range(1, 50).OrderBy(x => rnd.Next(1, 50)).Take(6).ToList();` – Jimi Aug 23 '19 at 21:35
  • 1
    @Jimi: Though that would work, it's not clear to me why you're using `rnd.Next(1, 50)` as your order key, instead of just `rnd.NextDouble()`. Is there something special about the numbers 1 to 50? Also, this solution becomes increasingly inefficient if the range gets a lot bigger than 50. – Eric Lippert Aug 23 '19 at 21:43
  • @Eric Lippert Not my solution. I just assembled what Steve proposed (I didn't change the method). `(1, 50)` is the range of values as seen in the question (also in Steve's answer). Not needed in `rnd.Next(1, 50)`, but Steve already fixed that. It's meant to fill the ListBox (the *slowest* part). Which also may not be what's needed if the ListBox already contains some objects. It's something the OP may find useful. – Jimi Aug 23 '19 at 21:57
  • `OrderBy` is stable, so the changes of smaller numbers with lower indexes and larger number at higher indexes is increased, and overall smaller numbers have a higher change to be included. – Residuum Aug 23 '19 at 22:10