12
\$\begingroup\$

I've written an abstract class in C# for the purpose of random number generation from an array of bytes. The .NET class RNGCryptoServiceProvider can be used to generate this array of random bytes, for example.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace MyLibrary
{
    /// <summary>
    /// Represents the abstract base class for a random number generator.
    /// </summary>
    public abstract class Rng
    {
        /// <summary>
        /// Initializes a new instance of the <see cref="Rng"/> class.
        /// </summary>
        public Rng()
        {
            //
        }

        public Int16 GetInt16(Int16 min, Int16 max)
        {
            return (Int16)(min + (Int16)(GetDouble() * (max - min)));
        }

        public Int32 GetInt32(Int32 min, Int32 max)
        {
            return (Int32)(min + (Int32)(GetDouble() * (max - min)));
        }

        public Int64 GetInt64(Int64 min, Int64 max)
        {
            return (Int64)(min + (Int64)(GetDouble() * (max - min)));
        }

        public UInt16 GetUInt16(UInt16 min, UInt16 max)
        {
            return (UInt16)(min + (UInt16)(GetDouble() * (max - min)));
        }

        public UInt32 GetUInt32(UInt32 min, UInt32 max)
        {
            return (UInt32)(min + (UInt32)(GetDouble() * (max - min)));
        }

        public UInt64 GetUInt64(UInt64 min, UInt64 max)
        {
            return (UInt64)(min + (UInt64)(GetDouble() * (max - min)));
        }

        public Single GetSingle()
        {
            return (Single)GetUInt64() / UInt64.MaxValue;
        }

        public Double GetDouble()
        {
            return (Double)GetUInt64() / UInt64.MaxValue;
        }

        public Int16 GetInt16()
        {
            return BitConverter.ToInt16(GetBytes(2), 0);
        }

        public Int32 GetInt32()
        {
            return BitConverter.ToInt32(GetBytes(4), 0);
        }

        public Int64 GetInt64()
        {
            return BitConverter.ToInt64(GetBytes(8), 0);
        }

        public UInt16 GetUInt16()
        {
            return BitConverter.ToUInt16(GetBytes(2), 0);
        }

        public UInt32 GetUInt32()
        {
            return BitConverter.ToUInt32(GetBytes(4), 0);
        }

        public UInt64 GetUInt64()
        {
            return BitConverter.ToUInt64(GetBytes(8), 0);
        }

        /// <summary>
        /// Generates random bytes of the specified length.
        /// </summary>
        /// <param name="count">The number of bytes to generate.</param>
        /// <returns>The randomly generated bytes.</returns>
        public abstract byte[] GetBytes(int count);
    }
}

Any suggestions for improvements would be welcome.

\$\endgroup\$
  • \$\begingroup\$ What is the purpose of the interdependent get() methods? \$\endgroup\$ – Michael K Feb 2 '11 at 0:50
  • \$\begingroup\$ @Michael: The overloads with min/max params require floating-point random numbers. They in turn require integral random numbers to generate. (I see no other way, since BitConverter.GetDouble would skew the distribution.) \$\endgroup\$ – Noldorin Feb 2 '11 at 0:59
  • \$\begingroup\$ Are you doing this as an exercise? The built-in random number generator already has this kind of functionality. So if this is just an exercise in interface design it can be reviewed. If this is a real attempt at providing a random number interface I think you have some more explaining to do before anybody can really provide any decent feedback. Note: Random number generation done correctly is a lot harder than you would think (i.e. it is very easy to screw up and get a bad distribution). \$\endgroup\$ – Martin York Feb 2 '11 at 8:03
  • 1
    \$\begingroup\$ @Martin York: The whole point of this is to provide an abstract contract for RNGs. System.Random does indeed provide this sort of functionality, but it is specific to an internal PNG (that has poor entropy). The idea is that implementations can derive from this class to implement specific RNG algorithms like the CSP one (RNGCryptoServiceProvider), Mersenne Twister, etc., simply by providing a generator for random bytes. The code above says nothing as to the random bytes are generates. \$\endgroup\$ – Noldorin Feb 2 '11 at 12:42
  • 1
    \$\begingroup\$ @TheXenocide: Also looking on this code I suppose maximum is exclusive here ;) \$\endgroup\$ – Snowbear Feb 2 '11 at 14:40
8
\$\begingroup\$

I think the design is pretty good. A few comments:

  • I'd rename the class to something a bit more descriptive, say RandomGenerator. Then when you implement the class you can declare it with CspGenerator: RandomGenerator or MersenneGenerator: RandomGenerator and it's obvious what the class does.

  • Comment the get() methods. IMO all public elements should be documented. Get/set could be left out, but that is a matter of preference. In particular I'd like to know what kind of range min anf max is and is used for.

  • Is getBytes() needed externally? If not, I would consider making it class-level rather than public.

The formatting is good - even in Visual Studio I've seen it get messed up as code is refactored and changed.

\$\endgroup\$
  • 1
    \$\begingroup\$ I concur with the class rename. Comments are always good and especially in reusable libraries, though I personally tend to forget to mention them when reviewing code, lol. Generally GetBytes is publicly available on random number generators. \$\endgroup\$ – TheXenocide Feb 2 '11 at 14:32
  • \$\begingroup\$ I think “Rng” is one of the very few cases where an abbreviation can actually be safely used (this may not be such a commending reference but the .NET framework does use it, after all). \$\endgroup\$ – Konrad Rudolph Feb 2 '11 at 16:49
  • \$\begingroup\$ Thanks for the answer. I agree with commenting the Get methods; in fact I did that some time after I posted this. :) You may have a point about the name, but like Konrad Rudolph I believe that RNG is a very common/understandable acronym. I wasn't sure about the modifier for GetBytes either, but I can foresee potential usage cases, so it doesn't hurt to expose it. \$\endgroup\$ – Noldorin Feb 2 '11 at 18:21
  • 2
    \$\begingroup\$ I think GetBytes is useful as a public method, for tasks too numerous to list (e.g. generating a key of some length, fuzz testing etc.) \$\endgroup\$ – dbkk Feb 2 '11 at 18:40
6
\$\begingroup\$

I think using GetDouble to generate the other random numbers can create performance problems when the user needs efficient random numbers.

Since GetBytes should return a uniform distribution anyway, can’t you bypass using floating-point numbers? See e.g. Java’s Random.nextInt implementation.

Something else, but this may be unnecessary and YAGNI for you: have you considered decoupling the RNG from the probability density function? At the moment your RNG directly supports generating uniformly distributed numbers from within a given range – but it supports no other distributions. This could be off-loaded into a separate Distribution class. For reference, Boost.Random does just that.

\$\endgroup\$
  • \$\begingroup\$ Ah yes, good point about the implementation for ranges. For some reason using the modulo operator did not occur to me! Regarding distributions, random numbers have to be generated according to some distribution to start - uniform is as good (and simpler) than any other. \$\endgroup\$ – Noldorin Feb 2 '11 at 18:19
  • \$\begingroup\$ @Noldorin: re distribution: of course. I was merely stating that this assumption existed anyway, so it could as well be used to generate the ranges. But in fact such a distribution shouldn’t be taken as granted. For example, LCGs exhibit different weaknesses (such as repeating lower bits, or unevenly distributed upper bits) so using either modulo or division must take that into account. The Java implementation does, but then it knows what kind of weakness the generated bytes have. Wikipedia has details (“Parameters in common use” table). \$\endgroup\$ – Konrad Rudolph Feb 2 '11 at 19:42
  • \$\begingroup\$ @Noldorin, it's not just performance: it's uniformity of distribution, which becomes a lot harder to guarantee once doubles get involved. I agree with using something à la Java's nextInt(int n). Note in particular that nextInt calculates, effectively (1<<32)%n and uses it to avoid slight bias in favour of numbers smaller than that. \$\endgroup\$ – Peter Taylor Feb 3 '11 at 17:14
  • \$\begingroup\$ @Peter: Well yes, indeed. Funnily, I think the above code is how System.Random does it (haven't checked Reflector with it though). I agree though, it's not ideal, and could potentially introduce skew. \$\endgroup\$ – Noldorin Feb 4 '11 at 0:21
3
\$\begingroup\$

If you're planning on placing this in a reusable library you should validate inputs (min > max throws an IndexOutOfRangeException, etc.) Also, you do not need to cast to double in the GetDouble method as the division implicitly returns a double and casting the first operand of the division in GetSingle still causes the division to return a double though you may be sacrificing some precision in the randomness as a result of sacrificing 32bits before you divide.

Otherwise the code does seems as though it would be sufficient. Depending on the scope of your solution perhaps you want to consider min/max overloads for GetSingle and GetDouble and if you're really looking to be special maybe support for System.Numerics.BigInteger and System.Decimal?

\$\endgroup\$
  • 1
    \$\begingroup\$ Just thought of this one, and it's no biggie really, but perhaps you want to provide a virtual void GetBytes(byte[] buffer, int offset, int length) with a default implementation. It's a very common pattern in code that uses byte arrays and implementations may leverage their underlying APIs to make this more efficient (no unnecessary allocation/GC, no need for them to copy data from one array to another, etc) \$\endgroup\$ – TheXenocide Feb 2 '11 at 14:37
  • \$\begingroup\$ @TheXenoicde: Your points are all good ones I think. I should indeed be throwing ArgumentExceptionss, and that overload for GetBytes would probably be handy too. Thanks! \$\endgroup\$ – Noldorin Feb 2 '11 at 18:22

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.