Ineffective Array Shuffler

Question

I am attempting to shuffle an array, but the way I am doing it only works about every fifth time. I would greatly appreciate if someone could explain why it is not working properly and perhaps propose a tweak.

private Button[] scrambleBoard(Button[] buttons)
{
    for (int x = 100 * buttons.Count(); x > 0; x--)
    {
        Random rand = new Random();
        int first = rand.Next(buttons.Count());
        int second = rand.Next(buttons.Count());


        Button temp = buttons[first];
        buttons[first] = buttons[second];
        buttons[second] = temp;
    }

    return buttons;
}

Answer 1

Move the following line outside the loop:

Random rand = new Random();

The default seed used by System.Random is based on Environment.TickCount. In a tight loop, the tick-count may not change between successive iterations, so it may end up using with the same seed over and over. Consequently, the loop will repeatedly swap the same two elements until the tick-count changes (it may not do so before the loop completes). To verify that this is the issue, you can try adding a Thread.Sleep(100) or similar inside the loop; you should then be able to see the shuffle working correctly (albeit very slowly).

You should also note that the technique you're using to permute the array is biased; not every permutation is equally likely. You might want to use a shuffling algorithm that is known to be unbiased, such as the Fisher-Yates shuffle.

Alternatively, you can use a really simple technique to shuffle. It's slightly inefficient, but unbiased:

var rand = new Random();
return buttons.OrderBy(button => rand.Next()).ToArray();

Answer 2

The problem is that you're creating the Random() object in each iteration of your loop. As the Random object uses a seed during initialization you'll find that most values will be identical and not random.

You can fix the problem by declaring the Random class as static outside the method body.

private static Random rand = new Random();

private Button[] scrambleBoard(Button[] buttons)
{
    for (int x = 100 * buttons.Count(); x > 0; x--)
    {
        int first = rand.Next(buttons.Count());
        int second = rand.Next(buttons.Count());


        Button temp = buttons[first];
        buttons[first] = buttons[second];
        buttons[second] = temp;
    }

    return buttons;
}

Answer 3

Your question has been answered, but I thought I'd share a nice little trick for shuffling a collection using LINQ and Guid. This creates a randomly ordered list with a nice spread of values.

private Button[] scrambleBoard(Button[] buttons)
{
    return buttons.OrderBy(b => Guid.NewGuid()).ToArray();
}