5
\$\begingroup\$

I have ported the code from my previous question "Generate a random numbers class" to C++14 and made a few modifications.

How can I improve it further?

#include <iostream>
#include <random>

template<typename T>
class Random
{
    template <typename U>
    static auto dist() -> typename std::enable_if_t<std::is_integral<U>::value, std::uniform_int_distribution<U>>{};

    template <typename U>
    static auto dist() -> typename std::enable_if_t<std::is_floating_point<U>::value, std::uniform_real_distribution<U>>{};


public:
    Random()
        : mRandomEngine(std::random_device()())
    {}

    auto operator()(T max)
    {
        decltype(dist<T>()) uniformDistribution(0, max - 1);
        return uniformDistribution(mRandomEngine);
    }

    auto operator()(T min, T max)
    {
        decltype(dist<T>()) uniformDistribution(min, max);
        return uniformDistribution(mRandomEngine);
    }

private:
    std::mt19937            mRandomEngine;
};

int main()
{
    Random<int> random;
    for (int i = 0; i < 9; ++i)
        std::cout << random(1, 8) << '\n';

    Random<float> randomf;
    for (int i = 0; i < 9; ++i)
        std::cout << randomf(1.f, 8.f) << '\n';
}
\$\endgroup\$
6
\$\begingroup\$

Asserting our preconditions

Your class requires T to either be an integral type or a floating point type. If it's not, you'll get a compile error at point of use on operator() instead of at point of declaration. Let's fix that:

static_assert(std::is_integral<T>::value || std::is_floating_point<T>::value, "!");

Unnecessary Complexity

We are conditionally selecting a distribution type based on whether or not T is integral or floating point. Once we assert that it's one or the other, we can simply use std::conditional:

using dist_type = std::conditional_t<
                      std::is_integral<T>::value,
                      std::uniform_int_distribution<T>,
                      std::uniform_real_distribution<T>>;

And just use that:

T operator()(T min, T max)
{
    dist_type uniformDistribution(min, max);
    return uniformDistribution(mRandomEngine);
}

This is a lot more direct. No extra template arguments. Also auto on return type is best reserved for those cases where you need it. This is a random engine generating Ts, so it really had better return a T.

Single-value?

Consider the difference between our distributions. uniform_int_distribution is closed:

Produces random integer values i, uniformly distributed on the closed interval [a, b]

but uniform_real_distribution is open:

Produces random floating-point values i, uniformly distributed on the interval [a, b)

This suggests one of two potential implementations

  1. Pass this difference onto your users. They should be aware that for floating point Ts, it will be open on the top end.
  2. Make your implementation always open on top end of the distribution.

Right now, we're half and half. It's fairly unexpected that:

Random<int> r;
r(1, 6); // can return 6
r(0, 6); // can return 6
r(6);    // cannot return 6

I'd recommend option (1):

T operator()(T max) {
    return (*this)(0, max);
}

Prefer Braces

Where you have:

: mRandomEngine(std::random_device()())

Prefer to brace-initialize the random_device:

: mRandomEngine(std::random_device{}())
|improve this answer|||||
\$\endgroup\$
2
\$\begingroup\$

If you leave out the default-constructor, your class will become an aggregate, and all members will be default-constructed, leading to the same result.

Though one could also explicitly initialize the random-generator differently at need.

|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.