8
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I missed a typo in the seed function that user miscco had brought up in their answer. I must of missed it because it is still valid code that will compile; it just won't execute as intended. It is now corrected. \$\endgroup\$ – Francis Cugler Mar 27 '18 at 13:24
6
\$\begingroup\$

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';
    }
}
\$\endgroup\$
  • \$\begingroup\$ I truly appreciate your feed back and what route you would take in the design process. However, my overall intentions would be that yes it is an interface that a user can use, but more importantly I'm also tailoring it within a much larger project that it will be use fairly extensively and that I may need different distribution types using different engines and may want to seed them in a different way depending on the context in which they are needed. I may at one point need a random seed, or at another point I might need a deterministic seed. \$\endgroup\$ – Francis Cugler Mar 28 '18 at 1:06
  • \$\begingroup\$ I was trying to make a simple header only file that would allow me to streamline any possible combination of a generator and distribution with various ways to seed them all from a simple class interface or function call. I found that the template class works better in this situation. \$\endgroup\$ – Francis Cugler Mar 28 '18 at 1:09
  • \$\begingroup\$ I could of made a simple class template wrapper specifically just for an engine, simply just for the distribution and one for the seed, then I could of wrote a function template that takes one of each; but I think their relationships go hand in hand so I opted for the class structure instead. \$\endgroup\$ – Francis Cugler Mar 28 '18 at 1:11
  • \$\begingroup\$ My overall project or library that I'm working on might end up having 4 or 5 different generator - distribution combinations and they could be generating millions of random numbers in a specified range. Some may be pure random while others might be deterministic. \$\endgroup\$ – Francis Cugler Mar 28 '18 at 1:12
  • \$\begingroup\$ @FrancisCugler: I don't pretend to know your needs better than you, of course. But, as I understand it, your library is a wrapper around <random> and ease of use is the added value of a wrapper. When I look at your Simple Use example, I fear that it is at least as complex as using <random> directly. Generator<std::mt19937, int, std::uniform_int_distriubtion> gen(engine, Seed::ChronoClock, 0, 1, 100 );: that's 2 template parameters and 5 function arguments, one of which is a custom type. Moreover I can't decide at first glance if the distribution bounds are 0 and 1, or 1 and 100. \$\endgroup\$ – papagaga Mar 28 '18 at 8:12
5
\$\begingroup\$

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>... ));
}
\$\endgroup\$
  • \$\begingroup\$ That was a missed typo; thank you for pointing that out. I fixed the if statement in the function call. However, my question still remains for the function. \$\endgroup\$ – Francis Cugler Mar 27 '18 at 13:22
  • \$\begingroup\$ I modified my code to match your function arguments and in my main I created a local lambda with capture by reference that all it does is takes a value for its parameter and multiplies it by 2 and returns the result. I then used my Generator's constructor and changed it to use UserDefined and instead of passing 0 for the seed value I passed in lam(val) where val was declared and initialized as an unsigned int with the value 5. My code compiled and works by passing it a lambda. \$\endgroup\$ – Francis Cugler Mar 28 '18 at 1:23
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Excellent feedback; I'll take that into consideration. \$\endgroup\$ – Francis Cugler Mar 29 '18 at 21:31

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.