HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Random Engine - follow up

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
randomenginefollow

Problem

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 
#include 

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

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

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

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

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

private:
    std::mt19937            mRandomEngine;
};

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

Solution

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::value || std::is_floating_point::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::value,
                      std::uniform_int_distribution,
                      std::uniform_real_distribution>;


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

  • Pass this difference onto your users. They should be aware that for floating point Ts, it will be open on the top end.



  • Make your implementation always open on top end of the distribution.



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

Random 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{}())

Code Snippets

static_assert(std::is_integral<T>::value || std::is_floating_point<T>::value, "!");
using dist_type = std::conditional_t<
                      std::is_integral<T>::value,
                      std::uniform_int_distribution<T>,
                      std::uniform_real_distribution<T>>;
T operator()(T min, T max)
{
    dist_type uniformDistribution(min, max);
    return uniformDistribution(mRandomEngine);
}
Random<int> r;
r(1, 6); // can return 6
r(0, 6); // can return 6
r(6);    // cannot return 6
T operator()(T max) {
    return (*this)(0, max);
}

Context

StackExchange Code Review Q#108932, answer score: 6

Revisions (0)

No revisions yet.