Geek Answers Handbook

Collection randomization using extension method

Possible duplicate:
C #: Does Random and OrderBy use a good shuffle algorithm?

I want to create an extension method that should mix elements in a collection.

Can I improve the following?

public static IList<T> RandomList<T>(this IList<T> source) { if (source.Count <= 0) throw new ArgumentException("No Item to Randomize"); for (int i =source.Count-1 ; i>0; i--) { int RandomIndex = Rnd.Next(i + 1); T temp = source[i]; source[i] = source[RandomIndex]; source[RandomIndex] = temp; } return source; }
+4
source share
5 answers

There are several questions that I would like to get with this method:

  • It should check the null argument.
  • It should not check a 0-length list.
  • Avoid side effects. Create a new list for shuffled items instead of modifying an existing one.
  • Do not hide dependencies. Pass the random number generator as an argument.
  • Use a more descriptive name than "RandomList".
  • The input type can be generalized to IEnumerable.
  • The method can be changed to a counter [generalize the type of output].

Essentially:

 public static IList<T> Shuffled<T>(this IEnumerable<T> source, Random generator) { if (source == null) throw new ArgumentNullException("source"); if (generator == null) throw new ArgumentNullException("generator"); //copy var result = source.ToList(); //shuffle the copy for (int i = result.Count - 1; i > 0; i--) { int RandomIndex = generator.Next(i + 1); T temp = result[i]; result[i] = result[RandomIndex]; result[RandomIndex] = temp; } return result; }

I did not generalize the type of output. You can do this if you want.

+1
source 
 public static IEnumerable<T> Shuffle<T>(this IEnumerable<T> source) { foreach(var item in source.OrderBy(i => Guid.NewGuid())) { yield return item; } }
+5
source

I think this is good enough if you know Random is not very random.

The Random class is viable for use in simple games and other non-scientific fields. Do not use it for cryptography.

+1
source

Generally, you should avoid modifying the list and return a new list instead. Even better would be to return IEnumerable so that it matches other extension and LINQ methods.

Try it.

 public static class RandomizeExtensionMethods { private static readonly Random _random = new Random(); public static IEnumerable<T> Randomize<T>(this IList<T> enumerable) { if (enumerable == null || enumerable.Count == 0) { return new List<T>(0); } return RandomizeImpl(enumerable); } public static IEnumerable<T> RandomizeImpl<T>(this IList<T> enumerable) { var indices = new int[enumerable.Count]; for(int i=0; i<indices.Length; i++) { indices[i] = i; } lock (_random) { for (int i = 0; i < indices.Length - 1; i++) { int j = _random.Next(i, indices.Length); int swap = indices[j]; indices[j] = indices[i]; indices[i] = swap; } } for(int i=0; i<indices.Length; i++) { yield return enumerable[indices[i]]; } } }
+1
source

Returning yourself is somewhat redundant. If you return a deep copy of the list, be sure; in which case it should be called "GetShuffledCopy ()" or something similar. If you are acting on the list itself, this should be an invalid return and called something like "Shuffle ()"

-Oisin

0
source

More articles:


All Articles