patterncsharpModerate
Randomly Permute Elements in a List
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.
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#?
/*===================================*
* 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
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
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.
This runs about 160 times as fast as your shuffling implementation.
Don't reinvent the wheel
When saving the results to disk, use
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.
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.