HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Randomly Permute Elements in a List

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
permuterandomlylistelements

Problem

I want to randomly permute a finite list in the most effective and efficient way in C#. My attempt is as follows.

/*===================================*
 * Compile it to produce Shuffle.exe *
 * ==================================*/
using System;
using System.Collections.Generic;
using System.IO;

namespace Shuffle
{
    class Program
    {
        static void Main(string[] args)
        {
            int Columns = int.Parse(args[0]);
            int Rows = int.Parse(args[1]);
            int Seed = int.Parse(args[2]);
            string OutputFilename = args[3];

            List OrderedList = new List();
            for (int x = 0; x  ShuffledList = new List();

            for (int i = 0; i < Columns * Rows; i++)
            {
                int x = rnd.Next(OrderedList.Count);
                ShuffledList.Add(OrderedList[x]);
                OrderedList.RemoveAt(x);
            }

            using (StreamWriter sw = new StreamWriter(OutputFilename))
            {
                foreach (string s in ShuffledList)
                    sw.WriteLine(s);
            }

        }
    }
}


This program will be used in my production. Could you review whether or not my code is already the most efficient and effective in C#?

Solution

Don't waste time on micro-optimizations. Use the simplest possible techniques that are known to perform well. In your specific case, it means replacing your multiple lists with one single array and using the standard fisher-yates-shuffle.
Naming conventions

In good C# code, local variables are camelCase beginning with a lower case letter. I renamed most of your variables to better express intent.

Generating the strings

You'll get a performance increase of about 30%-40% by using an array instead of a list (not much to be gained here).

Don't bother with string concatenation instead of string.Format, there is hardly any measurable difference in speed, but a huge difference in readability and maintainability.

string[] elements = new string[columns * rows];
for (int column = 0; column < columns; column++)
    for (int row = 0; row < rows; row++)
        elements[columns * row + row] = string.Format("{{{0},{1}}}", column, row);


Shuffling them

Whenever you remove from a list, all the remaining elements have to be shifted to fill the gap. Use your array instead and use the fisher-yates-shuffle; don't attempt inventing your own.

for (int i = elements.Length - 1; i > 0; i--)
{
    int swapIndex = random.Next(i + 1);
    string tmp = elements[i];
    elements[i] = elements[swapIndex];
    elements[swapIndex] = tmp;
}


This runs about 160 times as fast as your shuffling implementation.

Don't reinvent the wheel

When saving the results to disk, use File.WriteAllLines which does exactly what you were doing with the StreamWriter - in a single line.

File.WriteAllLines(outputFilename, elements);


Complete code (argument parsing omitted)

Once you've realized that a few milliseconds more or less don't matter, it would be good to split this up into several methods that each do exactly one thing. I'll leave that as an exercise to you.

string[] elements = new string[columns * rows];
for (int column = 0; column  0; i--)
{
    int swapIndex = random.Next(i + 1);
    string tmp = elements[i];
    elements[i] = elements[swapIndex];
    elements[swapIndex] = tmp;
}

File.WriteAllLines(outputFilename, elements);

Code Snippets

string[] elements = new string[columns * rows];
for (int column = 0; column < columns; column++)
    for (int row = 0; row < rows; row++)
        elements[columns * row + row] = string.Format("{{{0},{1}}}", column, row);
for (int i = elements.Length - 1; i > 0; i--)
{
    int swapIndex = random.Next(i + 1);
    string tmp = elements[i];
    elements[i] = elements[swapIndex];
    elements[swapIndex] = tmp;
}
File.WriteAllLines(outputFilename, elements);
string[] elements = new string[columns * rows];
for (int column = 0; column < columns; column++)
    for (int row = 0; row < rows; row++)
        elements[column*rows + row] = string.Format("{{{0},{1}}}", column, row);

Random random = new Random(seed);
for (int i = elements.Length - 1; i > 0; i--)
{
    int swapIndex = random.Next(i + 1);
    string tmp = elements[i];
    elements[i] = elements[swapIndex];
    elements[swapIndex] = tmp;
}

File.WriteAllLines(outputFilename, elements);

Context

StackExchange Code Review Q#15449, answer score: 14

Revisions (0)

No revisions yet.