7
\$\begingroup\$

Recently had reason to design a range based random number generator that uses the RNGCryptoServiceProvider provided by the .net api.

The basic algorithm generates a random unsigned 64 bit integer and creates a floating point number between 0 and 1 by dividing it by UInt64.MaxValue+ 1. This is then multiplied by the range and added to the minimum, to produce a 32 bit integer in the right range.

The class also includes a shuffle method.

using System;
using System.Collections.Generic;
using System.Security.Cryptography;
using System.Linq;
public static class CryptoRandom
{
    const double MAX_RANGE = (double)UInt64.MaxValue + 1;

    /// <summary>
    /// Get a cryptographic random integer in the range from
    /// min(inclusive) to max(exclusive)            
    /// </summary>
    /// <param name="min"></param>
    /// <param name="max"></param>
    /// <returns></returns>
    public static int Next(int min, int max)
    {
        if (max < min)
        {
            throw new ArgumentException("max is less than min");
        }
        if(min < 0)
        {
            throw new ArgumentException("min and max must be positive integers");
        }
        if (min == max)
        {
            return min;
        }
        using (RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider())
        {
            byte[] randomNumber = new byte[8];

            rng.GetBytes(randomNumber);
            double baseNum = BitConverter.ToUInt64(randomNumber, 0) / MAX_RANGE;

            int range = max - min;

            return (int)(baseNum * range) + min;
        }

    }
    /// <summary>
    /// Get a cryptographic random integer in the range from
    /// 0 to max(exclusive)
    /// </summary>
    /// <param name="max"></param>
    /// <returns></returns>
    public static int Next(int max)
    {
        return Next(0, max);
    }
    /// <summary>
    /// Get a cryptographic random 32-bit integer
    /// </summary>
    /// <returns></returns>
    public static int Next()
    {
        return Next(0, Int32.MaxValue);
    }
    /// <summary>
    /// A cryptographic random shuffle method
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <param name="input"></param>
    /// <returns></returns>
    public static ICollection<T> Shuffle<T>(ICollection<T> input)
    {
        return input.OrderBy(x => Next()).ToArray();
    }
}
\$\endgroup\$
4
\$\begingroup\$

When dealing with fair randomness and bit manipulation code, you have to be a lot more careful than you currently are.

Your random number generator is not fair, i.e. not all numbers have exactly the same possibility of being returned. Have a look at Java's implementation of java.util.Random or the Python standard library, which are all open source and good learning resources.

When you do integer calculations, don't use floating point calculations internally. They are very tricky to do correctly. Think about rounding for every arithmetic operation.

Shuffling is not the same as sorting. Read the Wikipedia article on shuffling (or any other article) to get this right.

Your Next() function is documented to return a 32-bit integer, but that integer has a little less than 31 bits of randomness. Therefore the documentation is misleading. To properly implement this method, you should get 32 bits of randomness from the underlying random generator, and if you only want positive numbers, return rnd & Integer.MaxValue. This gives you exactly 31 bits of randomness, and this is also what the documentation should say.

In the Next(min, max) function, you must throw an exception if min == max. Your current code returns max in that case, but the documentation says that max is exclusive.

And finally, remove redundant documentation comments. Having an empty <param name=min></param> comment is worthless, since it doesn't add any information. The code already says clearly that there is a parameter called min, and the comment should not repeat this. If you had written some additional information in that comment (like you did in the summary), it would be ok.

\$\endgroup\$
1
\$\begingroup\$

You should write more consistent exception messages. Currently the first one says why the exception occured, whereas the other one does not give any reason but instead it says what to do in order to avoid it in future. Giving a solution with the first message would be more useful.


@RollanIllig already said what is wrong with Shuffle. I'd like to add that even if it was ok to do it then there is no need for it to use either ICollection or ToArray. It would more linq-ish if you just used IEnumerable and let the user execute it when he needs to instead of doing it for him. This method also shouldn't be part of this class forcing the user to use your random numbers.

\$\endgroup\$

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.