5
\$\begingroup\$

I have written this code to shuffle a deck of cards. I would like to hear your inputs.

public static class Helper
{

    public static Int32 GetRandomNo(this RNGCryptoServiceProvider rng, byte[] data)
    {
        rng.GetBytes(data);
        var randomNo = BitConverter.ToInt32(data, 0);
        return randomNo;
    }

    public static void Shuffle<T>(this List<T> source)
    {
        using (RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider())
        {
            // Buffer storage.
            byte[] data = new byte[4];

            source =
                source.Select(element => new {element, randomValue = rng.GetRandomNo(data)})
                    .OrderBy(entry => entry.randomValue)
                    .Select(entry => entry.element)
                    .ToList();
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I'd suspect you've already seen this? \$\endgroup\$ – πάντα ῥεῖ Apr 12 '17 at 17:25
  • \$\begingroup\$ I have seen that thread, but the approach I am taking is I think different. So wanted to submit the approach for a review to see how good is it from say solns from that thread. :) \$\endgroup\$ – Sameer Apr 13 '17 at 2:14
4
\$\begingroup\$

4 bytes is only 4,294,967,296
Consider there are 52! possible shuffles = 8 * 10^67

Use a proper shuffle like Fisher Yates. You get a proper shuffle with only 0-51 random. Also shuffle from the prior shuffle (not a sorted deck). If you look closely Fisher Yates it produces exactly 52! deals. It is perfect.

I am not sure about your algorithm but I think the problem is that it will produce too many shuffles and they will not have uniform distribution. I think it will produce 52^52 deals.

A perfect random with a less than perfect algorithm is a bigger vulnerability than a perfect algorithm and a less than perfect random.

You can use the regular Random and a proper algorithm and not be exploited unless they know the seed. But it does not hurt to use RNGCryptoServiceProvider.

Poker security

\$\endgroup\$
  • \$\begingroup\$ Oops, I'm sorry, I misread the code. I thought that the total randomness would be 32 bits. \$\endgroup\$ – Roland Illig Apr 12 '17 at 22:05
  • \$\begingroup\$ Thanks for the wise words : "A perfect random with a less than perfect algorithm is a bigger vulnerability than a perfect algorithm and a less than perfect random." -:) \$\endgroup\$ – Sameer Apr 13 '17 at 4:49
1
\$\begingroup\$

I wonder that the linked SO question does not mention the Sort method. With it you can randomize the list in place by creating a custom Comparison<T>. You should also return the new result so you can chain other extensions if necessary.

public static List<T> Shuffle<T>(this List<T> source)
{
    using (var rng = new RNGCryptoServiceProvider())
    {
        var data = new byte[4];
        source.Sort(CompareRandomNumbers<T>(data, rng.GetRandomNo));            
        return source;
    }
}

private static Comparison<T> CompareRandomNumbers<T>(byte[] data, Func<byte[], int> getRandomNumber)
{
    return (x, y) => getRandomNumber(data).CompareTo(getRandomNumber(data));
}

Usage:

var randomizedList = new[] { "a", "b", "c", "d" }.ToList().Shuffle();
\$\endgroup\$
  • \$\begingroup\$ Doesn't this code violate the contract for CompareTo? \$\endgroup\$ – Roland Illig Apr 12 '17 at 21:29
  • \$\begingroup\$ @RolandIllig in which way? We are not interested in comparing the actual elements because we randomizing the list so instead we compare random numbers. \$\endgroup\$ – t3chb0t Apr 13 '17 at 4:06
  • \$\begingroup\$ It looks as if getRandomNumberis called more than once for each element. If so, CompareTo doesn't describe an ordering anymore. \$\endgroup\$ – Roland Illig Apr 13 '17 at 16:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.