Generators and Distributions 2.0

Question

In my original question: Generically encapsulating random generators and distributions into a class template I was given some excellent suggestions from two very informative users. I will give a basic outline of what each user had suggested by topic. I will finally demonstrate what changes I have made to the class.

---

User - bipll: https://codereview.stackexchange.com/users/159614/bipll

• Visibility
• Initialization
• Overloading
• Return Values
• Too Many Copies

---

User - papagaga: https://codereview.stackexchange.com/users/157553/papagaga

• Scoped Enumerations
• Universal References And Perfect Forwarding
• The Interface Is Perfectible
• Various

---

Refactored Class

#ifndef GENERATOR_H
#define GENERATOR_H

#include <limits>
#include <chrono>
#include <random>
#include <type_traits>
#include <functional>

enum class Seed { SeedValue, ChronoClock, RandomDevice, SeedSeq, UserDefined };

template<class Engine, class Type, template<typename> class Distribution>
class Generator {
public:
    typedef class Distribution<Type> Distribution;

private:
    Engine _engine;
    Distribution _distribution;
    Type _value;

public:    
    template<class SeedValue, class... Params>
    explicit Generator( Engine&& engine, Seed seedType, SeedValue seedValue, Params&&... params ) : _engine( std::move( engine ) ) {
        _distribution = Distribution( std::forward<Params>( params )... );    
        seed( seedType, seedValue );
    }

    Type generate() {
        return _value = _distribution( _engine );
    }

    Distribution getDistribution() const {
        return _distribution;
    }

    Type lastGeneratedValue() const {
        return _value;
    }

private:
    template<class SeedValue = int>
    void seed( Seed seedType, SeedValue value = 0 ) {
        if( seedType == Seed::SeedValue )
            _engine.seed( value );
    }

    // This overload has to choose between one or the other based on type passed
    void seed( Seed seedType ) {
        if( seedType == Seed::ChronoClock )
            _engine.seed( getTimeNow() );

        if( seedType == Seed::RandomDevice ) {
            std::random_device rd{};
            _engine.seed( rd() );
        }
    }

    void seed( Seed seedType, std::initializer_list<std::size_t>&& list ) {
        if( seedType == Seed::SeedSeq ) { // expects initializer_list<size_t>
            std::seed_seq seq( std::move( list ) );
            _engine.seed( seq );
        }
    }

    // NOTE: Not sure if this is correct; or how one would call it from current constructor
    template<class Ret, class... Args>
    void seed( Seed seedType, Args&&... ) {
        if( seedType == Seed::UserDefined ) { // can be any user defined method, lambda function<>, etc.
            std::function<Ret( std::forward<Args>... )> func;
            _engine.seed( func );
        }
    }

    std::size_t getTimeNow() {
        using Clock = std::conditional_t<std::chrono::high_resolution_clock::is_steady,
            std::chrono::high_resolution_clock,
            std::chrono::steady_clock>;

        std::size_t now = static_cast<std::size_t>(Clock::now().time_since_epoch().count());
        return now;
    }    
};

#endif // !GENERATOR_H

---

Simple Use

#include <iostream>
#include "Generator.h"

int main() {
    std::mt19937 engine;
    Generator<std::mt19937, int, std::uniform_int_distriubtion>
    gen( std::move( engine ), Seed::ChronoClock, 0, 1, 100 );

    for ( std::size_t i = 0; i < 100; i++ )
        std::cout << gen.generate() << '\n';

    std::cout << "\nPress any key and enter to quit.\n";
    std::cin.get();
    return 0;
}

---

Overview Of Changes Made From Each User's Suggestions

User - bipll:

Visibility: I moved the using Clock =... out of the public scope of class declaration and moved it specifically to the function body of the member getTimeNow() which is now a private method instead of a public one. As for the typedef of Distribution<Type> I added a public typedef to the class.

Initialization: I no longer have the construction of the engine type that is chosen by the user being constructed first then forcing them to have to explicitly call the seed function. The seed function is no longer public it is now private and is called by the class's constructor. I combined both construction and initialization into one process.

Overloading: I took the advise of overloading which came from both users and also allowed for an alternative overload that would accept a user defined mechanism to seed the generators.

Return Values: Before I had posted my original question and I had already though about having the generate function return the value, but I never incorporated that since I was only testing the class's functionality of proper construction and output. Now the generate() method returns a value. As for my previous method that use to get the generated value, I renamed it to be more intuitive lastGeneratedValue().

Too many copies: I have now incorporated the mechanics to use either perfect forwarding and or move semantics. I now move the engine into the constructor. This also reflects on user papagaga's section of Universal References And Perfect Forwarding

User - papagaga:

Scoped Enumerations: I changed my enum to be a class enum and also changed the field's case from SOME_FIELD to SomeField.

Universal References And Perfect Forwarding: Same as above; Using References & Perfect Forwarding I now forward the variadic parameters into the constructor.

The Interface Is Perfectible: This section is in reflection to the overload section seen from user bipll. I chose to use function overloads, and function template overloads where needed. I however did not include any static asserts.

Various: This pertained to the switch statement's braces being superfluous. This should not be an issue any more since I'm no long using a switch in replace of function overloading.

---

Questions - Concerns

I would like to know from both users who have provided a solid, well detailed, and informative answer if the changes I've made reflect what was suggested. Is the code accurate or does it still need some improvement?

As for my next question; it deals with the seed() overloaded function template. It is supposed to allow a user to define their own function object to be passed to the seed function in order to seed the generators or engines. This can be answered by anyone who feels that they have critical feedback. The function is untested, the code does compile and run without trying to use it. Does the syntax for the function declaration look correct? If so, kind of a stack overflow question but, how would I integrate the needed parameter(s) for this function into the this class's constructor? This particular function overload has me held up.

---

Note - Concerning my original post: I am divided and can not choose between either of the two users mentioned in this post as to who to accept for an accepted answer; both were extremely helpful. I up voted both users answers, yet I'm still undetermined on who to accept.

Answer 1

About the seed function, though, I must warn you that you might be surprised at some point by which overload the compiler will choose. Unless you really know what you're doing, having several template overloads, one of which exposes a default type, looks dangerous. If you multiply the overloads, the if constexpr c++17-feature is a better way to go, because it gives you a lot more control, and a lot more flexibility when you decide to extend your function.

I've looked a bit more into c++11's new random generators and the interface they expose. The std's engines' seed function takes either a value, or a sequence of values as a seed. I understand that you want to offer more options, but I fear you do it at the expense of usability (I'll need to remember that your seed function has more overloads than std's, and I'll need to remember also the different values of the Seed enum). You want to keep your interface orthogonal: what would it mean here? One simple seed function taking either a value or a range (let's say it's the x-axis) and as many functions as we need to generate the said value or range (that's our y-axis).

x-axis: a simple hook to the chose Engine::seed function is enough:

template <typename Seed_type>
void seed(Seed_type&& inp) {
    _engine.seed(std::forward<Seed_type>(inp));
}

y-axis: you can provide a simplified access to some convenient seed-generator functions, so that you end up calling seed that way:

gen.seed(current_time()); // your getTimeNow() function
// or
gen.seed(random_number());
// of if user defined
gen.seed(my_seed_function()); // you don't even need to check if it returns
                              // a value or a sequence

Those utility functions could have been static functions but that isn't really convenient in heavily templated classes. You could group them in a sub-namespace to make their role and their origin more obvious:

namespace seed {
    std::size_t current_time();
    std::size_t random_number();
    // ...
}
// ...
gen.seed(seed::random_number()); 

EDIT: I've given more thoughts about how I would have designed a wrapper around <random>. I like to start from use-cases, and I believe that the most frequent one is: "I need a random double", or "I need a random int between 1 and 6". This would lead to:

template <typename T>
T random(T low = min<T>, T high = max<T>);
// ...
auto dice_roll = random<int>(1, 6);

where:

template <typename T>
constexpr T min = std::numeric_limits<T>::min();
template <typename T>
constexpr T max = std::numeric_limits<T>::max();

Of course, it would mean creating a random_device, an engine and a distribution each time the function is called. It's okay if you call it a couple of times in your program, but you need to provide another interface for someone who wants n random integers.

The main point of my design would be to hide complexity. I wouldn't bother my client with seeds and distributions. They would choose a type, and optionally a range, but nothing more. If they want full control, they can use <random> directly. I take responsibility of selecting the best option for most cases, and I choose: 1) std::mt19937 is considered as the best all-arounder engine, 2) uniform distributions are what people usually expects, and 3) std::random_device was made to provide a seed.

I think you also need to provide a way to reset / recalibrate your generator.

So here's what I would have done:

#include <iostream>
#include <random>
#include <limits>
#include <type_traits>

// only stylistic
template <typename T>
constexpr T min = std::numeric_limits<T>::min();
template <typename T>
constexpr T max = std::numeric_limits<T>::max();

template <typename T>
class Generator {
    static_assert(std::is_arithmetic_v<T>); // <=> std::is_integral || std::is_floating_point

    public:
    using engine = std::mt19937;
    using distribution = std::conditional_t<std::is_integral_v<T>, 
                                            std::uniform_int_distribution<T>,
                                            std::uniform_real_distribution<T>>;
    public:
    Generator(T low = min<T>, T high = max<T>) :  
                                            _low(low), 
                                            _high(high), 
                                            _engine(std::random_device()()),
                                            _distribution(low, high)
                                            {}
    T random() { return _distribution(_engine); }
    void recalibrate(T low = min<T>, T high = max<T>) {
        _low = low;
        _high = high;
        _engine.seed(std::random_device()());
        _distribution = distribution(low, high);
    }
    private:
    T _low, _high;
    engine _engine;
    distribution _distribution;
};

template <typename T>
T random(T low = min<T>, T high = max<T>) { // convenience function
    Generator<T> gen(low, high);
    gen.random();
}


int main() {
    Generator<int> gen;
    for (auto i = 0; i < 10; ++i) {
        std::cout << gen.random() << '\n';
    }
    gen.recalibrate(20, 30);
    std::cout << std::endl;
    for (auto i = 0; i < 10; ++i) {
        std::cout << gen.random() << '\n';
    }
}

Answer 2

Your seed with a defined function seems wrong. THe first thing is you are missing the braces around the if and only the first statement is inside the if.

// NOTE: Not sure if this is correct; or how one would call it from current constructor
template<class Ret, class... Args>
void seed( Seed seedType, Args&&... ) {
    if( seedType == Seed::UserDefined ) // can be any user defined method, lambda function<>, etc.
        std::function<Ret( std::forward<Args>... )> func;
        _engine.seed( func );
}

Where is the function passed? Generally you would pass it as the second argument and then the argument pack

template<class Ret, class... Args>
void seed( Seed seedType, Ret&& func,  Args&&... ) {
    if( seedType == Seed::UserDefined )
        _engine.seed( func(std::forward<Args>... ));
}

Answer 3

Usage

80/20 rule. 20% of the library is used in 80% of the code. Optimize for common usage. In the usage case shown in the question, library could easily use an alias template:

template <typename Engine, typename Integral>
using uniform_generator = Generator<Engine, Integral, 
                                    std::uniform_distribution<Integral>>;

Example becomes

uniform_generator<std::mt19937, int> gen{std::move(engine), Seed::ChronoClock, 0, 1, 100};
for ( std::size_t i = 0; i < 100; i++ )
    std::cout << gen.generate() << '\n';

std::cout << "\nPress any key and enter to quit.\n";
std::cin.get();

Still a bit long though.

Define usage

From the example, it seems like the main usage scenario is to stream random numbers or generate some of them at once. Yet interface doesn't allow that. Make interfaces easy to use correctly and hard to use incorrectly, Scott Meyers (not sure though if he was the first one).

Undefined behavior

Documentation mentions presence of undefined behavior of Type is not one of the listed ones. Putting at least static_assert would be great.

Consider future

Usually there should be some reproducible sequence for debugging purposes. There might be engines that are not moveable, and some other small issues.

Too small seed

One 64 bit number is too small to seed ~20k bits of state. I prefer to use three numbers.

• duration_cast<nanoseconds>(high_resolution_clock::now()).count()

• std::random_device{}()

• number of your choice. Mine is 31

All of this thrown into std::seed_seq to eliminate bias.

It is much harder than it might look

In general, uniform distribution is not the ultimate distribution. Depending on the usage, other distribution might be needed. Discrete distributions could be used to balance the game dynamically, by moving weights around. Normal distribution might be used to get more of simple enemy NPC types and in other cases spawn one of the special ones. If you're sure you won't need the other ones, then that's great.

They don't have much in common

By looking at the documentation, it is clear that the only thing they share is their operator() and general value semantics. Construction and data retrieval are very different, aside from their behavior. Usually it is better to make a special code for the one that you use often, and use others directly. It is very hard to find an interface that would fit them all.