patterncppMinor
Random Engine - follow up
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?
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
Unnecessary Complexity
We are conditionally selecting a distribution type based on whether or not
And just use that:
This is a lot more direct. No extra template arguments. Also
Single-value?
Consider the difference between our distributions.
Produces random integer values i, uniformly distributed on the closed interval [a, b]
but
Produces random floating-point values i, uniformly distributed on the interval [a, b)
This suggests one of two potential implementations
Right now, we're half and half. It's fairly unexpected that:
I'd recommend option (1):
Prefer Braces
Where you have:
Prefer to brace-initialize the
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 6I'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 6T operator()(T max) {
return (*this)(0, max);
}Context
StackExchange Code Review Q#108932, answer score: 6
Revisions (0)
No revisions yet.