5
\$\begingroup\$

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;
    };
}
\$\endgroup\$
11
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Would PCG be a better choice when in need of randomness? \$\endgroup\$ – yuri Mar 8 '18 at 22:34
  • \$\begingroup\$ @yuri, I don't have experience in the area, but IIRC if the predefined random number generators are seeded well, they should be equivalent in numbers they produce. Just make to sure to seed with current time in nanoseconds, std::random_device{}(), and some number of your choice. That should avoid nasty behavior of mingw producing the same number all the time when std::random_device{}() is used. The approach requires use of std::seed_seq, which for unknown reasons is terribly hard to use. \$\endgroup\$ – Incomputable Mar 9 '18 at 9:37
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer. The reason the class is a template is so that IntRandom can be used for other types of integer (short, long, etc). \$\endgroup\$ – Ioan Thomas Mar 9 '18 at 13:47
  • \$\begingroup\$ I wouldn't suggest re-seeding your PRNG each time through the loop! That would be obviously bad. But you could do std::mt19937 g(...); for (size_t i=0; i < something large; ++i) { int i = std::uniform_int_distribution<int>(17, 42)(g); } and pay no performance penalty at all for doing so. \$\endgroup\$ – Quuxplusone Mar 9 '18 at 19:11
  • \$\begingroup\$ True but if you want to embed it somewhere such a simple convenience class is much better than writing it out \$\endgroup\$ – miscco Mar 9 '18 at 19:32

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.