Pseudo-random number generator implementation check

Question

I was searching for a pseudo-random number generator for C# and stumbled upon Tales from the CryptoRandom by Stephen Toub and Shawn Farkas, so I tried to implement a variation of their code.

The main difference with their implementation is mine returns inclusive ranges (e.g [0,1] and [min,max] instead of [0,1) and [min,max)),

namespace Albireo.SecureRandom
{
    using System;
    using System.Diagnostics.Contracts;
    using System.Security.Cryptography;

    public static class SecureRandom
    {
        private static readonly RNGCryptoServiceProvider Generator = new RNGCryptoServiceProvider();

        public static byte GetByte()
        {
            var buffer = new byte[1];
            Generator.GetBytes(buffer);
            return buffer[0];
        }

        public static double GetDouble()
        {
            var buffer = new byte[8];
            Generator.GetBytes(buffer);
            return BitConverter.ToDouble(buffer, 0);
        }

        public static short GetInt16()
        {
            var buffer = new byte[2];
            Generator.GetBytes(buffer);
            return BitConverter.ToInt16(buffer, 0);
        }

        public static short GetInt16(short minimum, short maximum)
        {
            Contract.Requires<ArgumentException>(minimum < maximum, "minimum < maximum");

            Contract.Ensures(Contract.Result<short>() >= minimum, "result >= minimum");
            Contract.Ensures(Contract.Result<short>() <= maximum, "result <= maximum");

            return (short) (minimum + (Sample() * (maximum - minimum)));
        }

        public static int GetInt32()
        {
            var buffer = new byte[4];
            Generator.GetBytes(buffer);
            return BitConverter.ToInt32(buffer, 0);
        }

        public static int GetInt32(int minimum, int maximum)
        {
            Contract.Requires<ArgumentException>(minimum < maximum, "minimum < maximum");

            Contract.Ensures(Contract.Result<int>() >= minimum, "result >= minimum");
            Contract.Ensures(Contract.Result<int>() <= maximum, "result <= maximum");

            return (int) (minimum + (Sample() * (maximum - minimum)));
        }

        public static long GetInt64()
        {
            var buffer = new byte[8];
            Generator.GetBytes(buffer);
            return BitConverter.ToInt64(buffer, 0);
        }

        public static long GetInt64(long minimum, long maximum)
        {
            Contract.Requires<ArgumentException>(minimum < maximum, "minimum < maximum");

            Contract.Ensures(Contract.Result<long>() >= minimum, "result >= minimum");
            Contract.Ensures(Contract.Result<long>() <= maximum, "result <= maximum");

            return (long) (minimum + (Sample() * (maximum - minimum)));
        }

        public static decimal Sample()
        {
            Contract.Ensures(Contract.Result<decimal>() >= 0, "result >= 0");
            Contract.Ensures(Contract.Result<decimal>() <= 1, "result <= 1");

            var buffer = new byte[8];
            Generator.GetBytes(buffer);
            return (BitConverter.ToUInt64(buffer, 0) & 0x7FFFFFFFFFFFFFFF) / (decimal) long.MaxValue;
        }
    }
}

I see there's already a similar question, but it lacks the actual implementation.

Since I'm not really skilled in mathematics, the main answers I'm looking for are:

• Is Sample()'s implementation correct? It should return a number between [0,1] by getting a random long, stripping its sign and dividing it by long.MaxValue to obtain the result.
• Is the computation performed in the various "ranged" methods correct? Usually to get a random value between min and max you do min + (rnd * (max - min + 1)) because rnd is [0,1), but here it's [0,1].
• Is there a way to have Sample() return a value between [0,1) instead of [0,1] to tons of code or wasting of "incorrect" values? Do note that the code is compiled with the /checked flag so arithmetic overflows/underflows throw an exception.
• How do I test a PRNG? By definition its results are... random, i.e. I can test GetInt16(short, short) as long as I want, but the numberOfGeneratedTestValues+1 call could still return an illegal value.

Any other comment is still appreciated.

Notes:

• I do know there is no XML documentation (yet).
• The class and its methods are static because RNGCryptoServiceProvider is thread safe, so there should be no reason to not have them static.

Answer 1

Your code is buggy in the range it pulls from the Sample() method. The easiest way to describe this bug is with the code: public static int GetInt32(int minimum, int maximum). For example, if I run:

GetInt32(0, 1)

I would expect 0 half the time, and 1 the other half.

Unfortunately, this is the implementing code:

return (int) (minimum + (Sample() * (maximum - minimum)));

which translates to:

return (int) (0 + (Sample() * (1)));

or, simply:

return (int)(Sample());

Now, sample is a Decimal value from 0.0 to 1.0 inclusive (assuming you got your Sample() method right...)

The cast-to-int will do a decimal truncation on the Sample, and reduce it to an int of 1, if the Sample was 1, and 0 if the sample was anything else...... (somewhere from 0.0 inclusive to 1.0 exclusive).

Which means there is a \$\frac{long.MaxValue - 1}{long.MaxValue}\$ chance the value will be 0, and a \$\frac{1}{long.MaxValue}\$ chance it is 1.

you should run the example code, and see if you ever get the value 1 for: GetInt32(0,1).

I ran your code here on Ideone and it shows poor distribution....

Answer 2

Adding a second answer addressing a different problem in the code..... inclusive/exclusive ranges.

Let's consider your Sample() method. I believe there are a number of problems here:

1. The range is non-uniform: long values have up to 18 significant digits or so (19 for unsigned). decimal has as many as 29 significant digits. Since you are dividing a random long value by the max long value, you are essentially able to create about \$10^{18}\$ possible decimal outputs, but, there are actually \$10^{29}\$ actual possibilities. What this means is that your Sample() method will never produce the \$10^{10}\$ or so of the possible values.... that decimal can express, between each actual output. Let me express it this way, your code produces values like (using an example decimal representation...):

   0.00000
   0.00001
   0.00002
   0.00003
   ....
   

   but, between each of these, the actual decimal value is able to produce:

   0.000000000000001
   0.000000000000002
   0.000000000000003
   0.000000000000004
   0.000000000000005
   0.000000000000006
   

   i.e., between each decimal value you produce, there are 10,000,000,000 decimal values you can't produce.

2. any use-case of the Sample() method will need to have special handling for exactly 1.0 (if it ever actually happens). This is because there is no easy way to handle that value in real life.... it is a tiny fraction of the possible ranges, yet, it has a huge significance on the output.

Bottom line is that Sample() is not practical to return 1.0, and it is not producing values it should in the middle of the ranges.

Sample should return a double, not a decimal, and it should not return 1.0. You will need to do deeper analysis on the return value though to ensure all values in the 0.0 to 1.0 (exclusive) are equally weighted.

For Sample(), at least, the use of the inclusive range, is a problem. Don't do it.

If you have an exclusive range on the return value, there are benefits.

Unfortunately, if you return a double from Sample, you can't use it to create a range of long, because double does not have the same possible range and granularity as long...