Random class in C++

Question

I've very new to C++ and have been working through a tutorial and got as far as std::rand(). I was immediately horrified and rushed out to see if I could implement this as a service. This is my (pretty basic) non-cryptographically-secure implementation in the style of .Net's System.Random.

There are a number of things I'm unhappy about:

• Anemic object. TBH there isn't much implementation here. Am I just (wrongly) trying to crowbar c++ into my .Net way of thinking? Is it a redundant layer of abstraction on top of the more powerful <random> library. However, my goal is to pass around one service for random numbers (to absolutely avoid hard coding things dependencies from <random> in my classes) and I'm not sure this can be achieved using <random> directly.

• Lifetime of the distributions. I'm not really sure what this should be. I've gone for the lifetime of the Random object where possible, but in the methods where I take a max-min value for each call this has not been possible. I'm pretty sure std::mt19937 should have the same lifetime as my Random object.

• Hard-coded uniform distributions. Preferably I would like some kind of IOC, it would be nice to pass in whether I'm using a uniform, poisson etc. I've looked inside the class and there a lot of templates, could I somehow wrap this into factory class that has methods to return int-returning-distributions, double-returning-distributions etc (would that even be a good idea?)? Given that System.Random has a hardcoded unif dist, arguably this may to too much IOC.

I think I've found an implementation that uses templates but I'm not really sure it's solving the IOC problem.

Interface: IRandom.h (Has no corresponding .cpp file)

#pragma once
class IRandom
{
public: 
    virtual int32_t Next() = 0;
    virtual int32_t Next(int32_t maxValue) = 0;
    virtual int32_t Next(int32_t minValue, int32_t maxValue) = 0;
    virtual double_t NextDouble() = 0;
    virtual double_t NextDouble(double_t minValue, double_t maxValue) = 0;
    virtual void NextBytes(std::vector<uint8_t>& buffer) = 0;
};

Random.h

#pragma once
#include <random>
#include "IRandom.h"
class Random : public IRandom
{
    std::mt19937 _randomNumberGenerator;
    std::uniform_real_distribution<double_t> _realDistribution;
    std::uniform_int_distribution<int32_t> _bytedistribution;
public:
    Random(uint_least32_t seed);
    Random() : Random(std::_Random_device()) {};
    ~Random();
    int32_t Next();
    int32_t Next(int32_t maxValue);
    int32_t Next(int32_t minValue, int32_t maxValue);
    double_t NextDouble();
    double_t NextDouble(double_t minValue, double_t maxValue);
    void NextBytes(std::vector<uint8_t>& buffer);
};

Random.cpp

#include "stdafx.h"
#include "Random.h"
#include <random>

Random::Random(uint_least32_t seed)
{
    this->_randomNumberGenerator = std::mt19937(seed);
    this->_realDistribution = std::uniform_real_distribution<double_t>();
    this->_bytedistribution = std::uniform_int_distribution<int32_t>(0, 256);
}

int32_t Random::Next()
{
    return this->Next(0, INT32_MAX);
}

int32_t Random::Next(int32_t maxValue)
{
    return this->Next(0, maxValue);
}

int32_t Random::Next(int32_t minValue, int32_t maxValue)
{
    std::uniform_int_distribution<int32_t> distribution(minValue, maxValue);
    return distribution(this->_randomNumberGenerator);
}

double_t Random::NextDouble()
{
    return this->_realDistribution(this->_randomNumberGenerator);
}

double_t Random::NextDouble(double_t minValue, double_t maxValue)
{
    std::uniform_real_distribution<double_t> distribution(minValue, minValue);
    return this->_realDistribution(this->_randomNumberGenerator);
}

void Random::NextBytes(std::vector<uint8_t>& buffer)
{   
    for (auto &i : buffer)
    {
        i = static_cast<uint8_t>(_bytedistribution(this->_randomNumberGenerator));
    }
}

Example Usage

unique_ptr<IRandom> randomService = make_unique<Random>();

cout << "NUMBERS" << endl;
for (int i = 0; i < 10; i++)
{
    cout << randomService->Next(0,10) << endl;
}

auto buffer = vector<uint8_t>(100);
randomService->NextBytes(buffer);

cout << "BUFFER" << endl;
for (auto &i : buffer)
{
    cout << i << endl;
}

Answer 1

Here are some comments mostly related to coding style and overall C++ practices (roughly in order of appearance):

1. Your interface is missing a virtual destructor. This is an important thing for an abstract interface and your code is actually invoking undefined behavior when trying to destroy an instance of IRandom without one. With C++11, the proper way of declaring a virtual destructor in a pure interface class is using the = default destructor idion:

   virtual ~IRandom() = default;
   

2. Using double_t is very unusual, since double is normally the same but with less typing. Unless you really have a good reason for double_t, you should stick to the native double instead. I would say the same about using int32_t. Unless you really care about using strictly sized 32bit integers, just go for a plain ol' int.

3. By the way, when using the sized types, like int32_t and friends, from <cstdint>, make sure to include the header file. You are relying on a residual include probably from <random>, which might not be there on other compilers or even on a future version of your current one. At any rate, it is also good practice making dependencies clear.

4. When using the types from <cstdint>, the correct way of referencing its members is thru the std:: namespace. They happen to also visible at the global level because on most compilers stdint.h (the C header) and cstdint (the C++ header) are implemented as wrappers to the same file. There is no requirement from the C++ Standard that this be implemented is such way, so a C++ compiler is not required to provide int32_t and friends outside the std namespace when including cstdint.

5. std::numeric_limits<T>::[min][max]() is preferred over the INT*_MAX / INT*_MIN macros.

6. Prefixing in-class member access with this-> is very rare in C++. You should only be doing that when, for instance, you need to disambiguate some shadowed member access (but even that is discouraged because shadowing members is a bit of a design flaw; better to uniquely name your variables and methods).

7. Identifiers beginning with underscore carry a few restrictions in C++ (Read this). Your use inside a class for member variables is not wrong, but might be frowned upon. Consider perhaps other styles, such as the m_prefix or and underscore atTheEnd_. Or no distinction at all for member variables.

8. I think you probably meant to camelCase _bytedistribution but mistyped the d in "distribution".

9. This is not the ideal way of initializing data in a constructor:

   Random::Random(uint_least32_t seed)
   {
       this->_randomNumberGenerator = std::mt19937(seed);
       this->_realDistribution = std::uniform_real_distribution<double_t>();
       this->_bytedistribution = std::uniform_int_distribution<int32_t>(0, 256);
   }
   

   That way you are calling the implicit default constructor and then reinitializing the members with the assignment operator. That's an easy mistake to make when both Java and C# use that syntax, but the proper way of initializing in a C++ constructor is by calling the constructor of the member object:

   Random::Random(uint_least32_t seed)
       : _randomNumberGenerator(seed)
       , _realDistribution() // <-- This one takes no arguments, so this line could even be omitted.
       , _byteDistribution(0, 256)
   { }
   

   Notice that it is a list of comma separated constructor calls, with the first starting at the :. This is called the constructor initialization list. C++11 also allows replacing the ( ) in the constructor calls by { }, so _randomNumberGenerator{ seed } would also be valid, if you happen to prefer that syntax.

10. While the virtual destructor in the interface definition is necessary, in your implementation class it is not. There is no reason for declaring an empty destructor but to making it a default virtual one. So you can just erase the destructor of Random if no manual cleanup is needed.

11. #include "stdafx.h" is very Visual Studio specific and I personally dislike it since it makes your code non-portable for free. If you could get rid of it, I don't see why you shouldn't (usually involves just deleting that line and adjusting a setting on the IDE to disable precompiled headers). Edit: This point aroused some discussion in the comments section, so let me start over. I'm not advocating against the use of precompiled headers, they can have their value on large projects. My problem with stdafx.h in this particular case and other small projects is that it is going to be the only thing that prevents the code from compiling elsewhere (say with GCC or Clang). If I copy-paste this, as it stands, into a couple files, I'll have to either remove that line or create an empty stdafx.h before I can build it, which is a nuisance, when the code could be ready to compile on other systems without any change if wasn't for this. Of course that if we are not talking about a code snippet in a website, but a source code tree distributed as a project, with a proper build system, this do not apply, since the build system should be properly configured to handle precompiled headers.

12. Add the new override specifier to methods of your implementation class (Random). This feature is meant to aid compiler diagnostics and provide better warning/error messages, as well as clearly communicating your intentions to human readers.

13. Regarding you tiny usage example, statically allocated object instances are usually not declared like this:

    auto buffer = vector<uint8_t>(100);
    

    But instead using this syntax:

    vector<uint8_t> buffer(100); 
    //  ^            ^      ^--- (optional) constructor call
    //  +--- type    |
    //               +--- variable
    

    Note: Herb Sutter seems to disagree (thanks @ChrisDrew for the comment). Personally, I am down for auto and type inference whenever it makes code shorter or easier to read. In this particular case, it is just more typing (which defeats one of the purposes of type inference, BTW). The constructor initialization syntax has been around since day 1, so it is pretty well rooted and known. The main argument for the auto var = Type() style would be to make things more uniform, however, when declaring a member variable, the type still has to go on the left hand side, so the argument for consistency is only partial.

Answer 2

Made some of the changes suggested by @glampert in his excellent answer. The code is here.

I've only annotated where I've differed really, so there's very little text. Most points have been incorporated, especially 9 and 13 (those were my favorite).

#pragma once
#include "stdafx.h"
#include <cstdint>
#include <vector>

namespace ConsoleApplicationCpp
{

    class IRandom
    {
    public:
        virtual ~IRandom() = default;
        virtual std::int32_t Next() = 0;
        virtual std::int32_t Next(std::int32_t maxValue) = 0;
        virtual std::int32_t Next(std::int32_t minValue, std::int32_t maxValue) = 0;
        virtual std::double_t NextDouble() = 0;
        virtual std::double_t NextDouble(std::double_t minValue, std::double_t maxValue) = 0;
        virtual void NextBytes(std::vector<std::uint8_t>& buffer) = 0;
    }; 

}

I however, do not trust the built in types and have decided to stick to <cstdint>, I have put in the std::s before these types and also decided to std:: rather than using namespace std, which I'm sure is a subject that has been discussed enough already

#pragma once
#include "stdafx.h"
#include <random>
#include <vector>

#include "IRandom.h"

namespace ConsoleApplicationCpp
{

    class Random : public IRandom
    {
        std::mt19937 randomNumberGenerator;
        std::uniform_int_distribution<std::int32_t> byteDistribution;
    public:
        Random(std::uint_least32_t seed);
        Random() : Random(std::_Random_device()) {};
        std::int32_t Next() override;
        std::int32_t Next(std::int32_t maxValue) override;
        std::int32_t Next(std::int32_t minValue, std::int32_t maxValue) override;
        std::double_t NextDouble() override;
        std::double_t NextDouble(std::double_t minValue, std::double_t maxValue) override;
        void NextBytes(std::vector<std::uint8_t>& buffer) override;
    };

}

No special "_" or "m_". I've made the call to break with that sort of naming in C# (not all do) and I think it's best to break with it here. This means I will be using this->member. I really like using this, and accept I am guilty of coming to C++ and demanding it be written like C#.

#include "stdafx.h"
#include <cstdint>
#include <random>

#include "Random.h"

namespace ConsoleApplicationCpp
{
    Random::Random(std::uint_least32_t seed)
        : randomNumberGenerator(seed),
        byteDistribution(0, 256)
    {
    }

    std::int32_t Random::Next()
    {
        return this->Next(0, std::numeric_limits<std::int32_t>::max());
    }

    std::int32_t Random::Next(std::int32_t maxValue)
    {
        return this->Next(0, maxValue);
    }

    std::int32_t Random::Next(std::int32_t minValue, std::int32_t maxValue)
    {
        if (minValue < 0 || maxValue < minValue)
        {
            throw 
                std::invalid_argument("minValue and maxValue must be non-negative. maxValue must be greater than minvalue");
        }
        std::uniform_int_distribution<std::int32_t> distribution(minValue, maxValue);
        return distribution(this->randomNumberGenerator);
    }

    std::double_t Random::NextDouble()
    {
        return this->NextDouble(0.0, 1.0);
    }

    std::double_t Random::NextDouble(std::double_t minValue, std::double_t maxValue)
    {
        if (minValue < 0.0 || maxValue < minValue)
        {
            throw
                std::invalid_argument("minValue and maxValue must be non-negative. maxValue must be greater than minvalue");
        }
        std::uniform_real_distribution<std::double_t> distribution(minValue, maxValue);
        return distribution(this->randomNumberGenerator);
    }

    void Random::NextBytes(std::vector<std::uint8_t>& buffer)
    {
        for (auto &i : buffer)
        {
            i = static_cast<std::uint8_t>(byteDistribution(this->randomNumberGenerator));
        }
    }

}

I've kept "stdafx.h" for now, mostly out of laziness, but I've stripped the includes out of it. I'm using it to contain some macros that cover the confusingness of __declspec, but I should probably split that into its own header file.