Collection Randomization using Extension Method [duplicate]

This question already has answers here: Closed 13 years ago.

Possible Duplicate:

C#: Is using Random and OrderBy a good shuffle algorithm?

I want to create an extension method which should shuffle the items in the 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;
 }

---

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

---

There are a few issues I would have with this method:

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

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 didn't generalize the output type. You can do that if you want.

---

I think is good enough as long as 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.

---

Generally you should avoid changing the list and instead return a new list. Even better would be to return IEnumerable to be consistent with other Extension methods and LINQ.

Try this.

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]];
        }
    }
}

---

Having it return itself is somewhat redundant. If you were returning a deep copy of the list, sure; and in that case it should get called "GetShuffledCopy()" or something similar. If you're acting on the list itself, it should be a void return and be called something like "Shuffle()"

-Oisin