2
\$\begingroup\$

I wrote a simple wrapper for the <random> header, as follows:

#include <random>

namespace Random
{

    static int getInt(int min, int max)
    {
        std::random_device rd;
        std::mt19937 gen(rd());
        std::uniform_int_distribution<> distribution(min, max);

        int rnd = distribution(gen);

        return rnd;
    }

    static float getFloat(float min, float max)
    {
        std::random_device rd;
        std::mt19937 gen(rd());
        std::uniform_real_distribution<> distribution(min, max);

        float rnd = distribution(gen);

        return rnd;
    }

}

I basically just want to know: is there a better / more efficient way to do it?

I know for a fact that it works quite well, but I am no expert. I thank you in advance for your answers.

\$\endgroup\$
3
\$\begingroup\$

You might be able to save the overhead of constructing a new generator on each call, with something like:

static std::mt19937 default_generator(std::random_device{}());

static int getInt(int min, int max)
{
    return std::uniform_int_distribution{min, max}(default_generator);
}

But, given that most uses require several values from a single distribution and there's rarely a need to use more than one generator, is this header really worth its cognitive load? Each function is a one-liner, and even less if the distribution can be re-used over several calls.

We'd also expect to have versions for longer or unsigned integer types, and for double and other floating-point types. It probably makes more sense for it to be a template:

#include <random>
#include <type_traits>

template<typename T>
inline T getUniformRandom(T min, T max)
{
    static std::mt19937 default_generator(std::random_device{}());
    if constexpr (std::is_integral_v<T>) {
        return std::uniform_int_distribution{min, max}(default_generator);
    } else if constexpr (std::is_floating_point_v<T>) {
        return std::uniform_real_distribution{min, max}(default_generator);
    }

    static_assert(0, "getUniformRandom requires an arithmetic type");
    return min+max;             // just to eliminate compiler warnings
}

Alternatively, use std::enable_if instead of if constexpr:

#include <random>
#include <type_traits>

template<typename T>
inline std::enable_if_t<std::is_integral_v<T>, T> getUniformRandom(T min, T max)
{
    static std::mt19937 default_generator(std::random_device{}());
    return std::uniform_int_distribution{min, max}(default_generator);
}

template<typename T>
inline std::enable_if_t<std::is_floating_point_v<T>, T> getUniformRandom(T min, T max)
{
    static std::mt19937 default_generator(std::random_device{}());
    return std::uniform_real_distribution{min, max}(default_generator);
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ The real problem is that it is intended that there is only one PRNG (or at least a small number of PRNGs) and random_device is intended to be called once to seed the PRNG, rather than creating new PRNGs each time. which is solved by static. \$\endgroup\$ – L. F. Jan 15 at 9:06
  • \$\begingroup\$ Yes, that's right. We might need one PRNG per thread, but certainly not one per call. \$\endgroup\$ – Toby Speight Jan 15 at 10:21
2
\$\begingroup\$

Just to expand on Toby's response, the two functions are basically

static ArithmeticType getAritmeticType(ArithmeticType min, ArithmeticType max)
{
    std::random_device rd;
    std::mt19937 gen(rd());
    UniformDistributionType distribution(min, max);

    ArithmeticType rnd = distribution(gen);

    return rnd;
}

All of the duplicate code leaps out while the arithmetic type and distribution type (which depends on the arithmetic type) change. An alias may be used to switch to the appropriate uniform distribution based on the provided arithmetic type.

template <typename ArithmeticType>
using uniform_distribution = typename std::conditional<
            std::is_integral<ArithmeticType>::value,
                std::uniform_int_distribution<ArithmeticType>,
                std::uniform_real_distribution<ArithmeticType>
            >::type;

template <typename ArithmeticType>
static ArithmeticType getAritmeticType(ArithmeticType min, ArithmeticType max)
{
    std::random_device rd;
    std::mt19937 gen(rd());
    return uniform_distribution<ArithmeticType>{min, max}(gen);
}

Side note: Consider how others will use this function.

  • There is nothing in the name getInt/Float that documents how the numbers are being generated (first? random?) or how distributed the possible results are. We already know it gets an int/float by its signature. What does the function actually do?
  • The user may not even want to use std::mt19937 or may want to reuse an existing psuedo-random bit generator.
  • The user may want to use a fixed seed for testing.
  • std::random_device may actually be deterministic depending on the sources of entropy an implementation uses. It can also throw at any time for all sorts of reasons.
  • Users are forced to use a poorly-initialized Mersenne Twister. std::random_device only produces a single 32-bit integer. A single 32-bit integer provides \$2^{32}\$ possible initialization states. * Mersenne Twister needs 624 32-bit integers of seed to be properly initialized, which is provided when you seed with std::seed_seq. The result is a generator that is both easily predictable and biased.

Consider allowing the user to pass the generator.

template <typename ArithmeticType>
using uniform_distribution = typename std::conditional<
            std::is_integral<ArithmeticType>::value,
                std::uniform_int_distribution<ArithmeticType>,
                std::uniform_real_distribution<ArithmeticType>
            >::type;

template <typename ArithmeticType, typename RandomBitGenerator>
ArithmeticType uniform_rand(RandomBitGenerator& gen,
                            ArithmeticType min,
                            ArithmeticType max)
{
    return uniform_distribution<ArithmeticType>{min, max}(gen);
}

I basically just want to know: is there a better / more efficient way to do it?

Yes. There are non-standard distributions and pseudo-random bit generators that are better/more efficient. Daniel Lemire published a paper in late 2018 going over a faster approach to generating random integers in a range.

| improve this answer | |
\$\endgroup\$

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.