C++ Random Number Generation

Question

I am making a few wrapper classes for the C++ standard library and was wondering if the following code is good practice.

#pragma once

#include <random>

namespace LibraryName
{
    class RandomBase
    {
    protected:
        RandomBase()
            : m_RandomGenerator(m_RandomDevice()) {}

        std::random_device m_RandomDevice;
        std::mt19937 m_RandomGenerator;
    };

    template<typename T>
    class IntRandom : public RandomBase
    {
    public:
        IntRandom(T minValue, T maxValue)
            : m_Distribution(minValue, maxValue), m_MinValue(minValue), m_MaxValue(maxValue) {}

        T Next()
        {
            return m_Distribution(m_RandomGenerator);
        }

        inline T GetMinValue() const { return m_MinValue; }
        inline T GetMaxValue() const { return m_MaxValue; }

    private:
        std::uniform_int_distribution<T> m_Distribution;
        T m_MinValue;
        T m_MaxValue;
    };

    template<typename T>
    class FloatRandom : public RandomBase
    {
    public:
        FloatRandom(T minValue, T maxValue)
            : m_Distribution(minValue, maxValue), m_MinValue(minValue), m_MaxValue(maxValue) {}

        T Next()
        {
            return m_Distribution(m_RandomGenerator);
        }

        inline T GetMinValue() const { return m_MinValue; }
        inline T GetMaxValue() const { return m_MaxValue; }

    private:
        std::uniform_real_distribution<T> m_Distribution;
        T m_MinValue;
        T m_MaxValue;
    };

    class BoolRandom : public RandomBase
    {
    public:
        BoolRandom() : BoolRandom(0.5f) {}
        BoolRandom(float trueChance)
            : m_Distribution(trueChance), m_TrueChance(trueChance) {}

        bool Next()
        {
            return m_Distribution(m_RandomGenerator);
        }

        inline bool GetTrueChance() const { return m_TrueChance; }

    private:
        std::bernoulli_distribution m_Distribution;
        float m_TrueChance;
    };
}

Answer 1

That seems like a lot of code for very little purpose. It's allowing you to write

IntRandom<int> r(17, 42);
int i = r.Next();

instead of the no-helpers version,

std::mt19937 g(std::random_device{}());
std::uniform_int_distribution<int> dist(17, 42);
int i = dist(g);

Some people would say that that's not worth the 77 lines of code.

---

Two small problems with your code, related to your use of random_device: First, you're using only 32 bits of entropy to seed your entire Mersenne Twister; it would be better to use as many bits as the mt19937 has bits of state. However, doing this correctly is needlessly difficult in C++17, so you get a pass on it for now.

Second, your RandomBase class keeps a member of type random_device. This might be problematic, because a random_device is essentially an open file handle to /dev/urandom. If you create a lot of RandomBase objects at once, you might find yourself running out of file handles.

Oh, and a third problem is that random_device is non-copyable and non-movable; so your RandomBase is also non-movable. That could actually be pretty bad, depending on how you want to use it.

---

You could fix the random_device issues by removing the random_device data member, and instead providing a method like this:

void RandomBase::reseed() {
    std::random_device rd;
    m_RandomGenerator.seed(rd());  // TODO: better seeding
}

This keeps the random_device alive (and /dev/urandom open) only as long as it's needed; and it removes the data member from the class so that the class becomes movable and even copyable.

---

You could also reduce duplication by using templates and type-aliases, something like this:

template<class Distribution>
class dist_and_gen {
    std::mt19937 m_g;
    Distribution m_dist;

    template<class... Args>
    explicit constexpr dist_and_gen(Args&&...) :
        m_dist(std::forward<Args>(args)...) {}

    auto next() { return m_dist(m_g); }
    auto params() const { return m_dist.params(); }
};

template<class T> using IntRandom =
    dist_and_gen<std::uniform_int_distribution<T>>;
template<class T> using FloatRandom =
    dist_and_gen<std::uniform_float_distribution<T>>;
template<class T> using BoolRandom =
    dist_and_gen<std::bernoulli_distribution<T>>;

This version loses some of the details, such as your custom-named accessors for the parameters; but it preserves a lot of the details "accidentally". For example, I didn't try to preserve your BoolRandom::BoolRandom() default constructor; but it turns out that it just works anyway, thanks to the default constructor of the standard bernoulli_distribution.

Answer 2

I basically wrote the same helper classes for my work. The advantage of them is twofold

1. It allows reproduction of noisy simulations by defining a seed

2. Normally random number generators are rather costly in their initialization but cheap in returning the next value. So regarding @Quuxplusone there is a major performance difference between

   IntRandom<int> r(17, 42);
   for (size_t i=0; i < something large; ++i)
       int i = r.Next();
   

   Compared to

   for (size_t i=0; i < something large; ++i){
       std::mt19937 g(std::random_device{}());
       std::uniform_int_distribution<int> dist(17, 42);
       int i = dist(g);
   }
   

   Also embedding them into your class is much easier with a given interface.

Now for the actual review. I think your base class lacks one key feature and that is passage of a seed. Also the random_device is unnecessary as @Quuxplusone pointed out, so reduce this to:

class RandomBase
{
protected:
    RandomBase() 
        : RandomDevice(std::mt19937(std::random_device()))
    {}

    RandomBase(const double seed) 
        : RandomDevice(std::mt19937(seed))
    {}
    std::mt19937 m_RandomGenerator;
};

Regarding your IntDistribution I would say it has a bad name and it is way overcomplicated. On one hand there is the member function std::uniform_int_distribution::max which does what your GetMaxValue does. Also i do not really see a benefit in it beeing a template class so I would reduce it to:

class RandomUniformInt : public RandomBase {
public:
    RandomUniformInt (int lower_bound, int upper_bound)
        : dist(lower_bound, upper_bound) 
    {}
    RandomUniformInt(int lower_bound, int upper_bound, double seed)
        : RandomBase(seed)
        , dist(lower_bound, upper_bound) 
    {}

    int operator ()() { return dist(m_RandomGenerator); }
private:
    std::uniform_int_distribution<> dist;
};

That said given the minimal impact of RadomBase I would actually inline it.