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';
}
2 Answers 2
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 T
s, 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
T
s, 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<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{}())
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.