8
\$\begingroup\$

I have made class to setup a random number bases on <random>. It works fine, but I have doubt in std::default_random_engine mEngine{ std::random_device()() }; Did i implement it correctly? What is the best practice for random?

#include <random>
#include <iostream>

// for some compilers that's don't support nested templates with the same parameter
template <typename T>
auto dist() -> typename std::enable_if<std::is_integral<T>::value, std::uniform_int_distribution<T> >::type;

template <typename T>
auto dist() -> typename std::enable_if<std::is_floating_point<T>::value, std::uniform_real_distribution<T> >::type;

template<typename T>
class Random
{
public:
    Random(const T& min, const T& max)
        : mUnifomDistribution(min, max)
    {}
    Random(const Random<T>&) = delete;
    Random(const Random<T>&&) = delete;
    Random<T>& operator = (const Random<T>&) = delete;
    T operator()()
    {
        return mUnifomDistribution(mEngine);
    }

private:
    std::default_random_engine mEngine{ std::random_device()() }; // <-- here the doubt - is it seeding?
    //template <typename T>
    //static auto dist() -> typename std::enable_if<std::is_integral<T>::value, std::uniform_int_distribution<T> >::type;

    //template <typename T>
    //static auto dist() -> typename std::enable_if<std::is_floating_point<T>::value, std::uniform_real_distribution<T> >::type;

    using type = decltype(dist<T>());
    type mUnifomDistribution;
};

int main() 
{
    ::Random<int> getRandom(0, 9);
    for (int i = 0; i<9; ++i)
        std::cout << getRandom() << '\n';
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You must test it to make sure it's implemented correctly. We can review it after it's confirmed to work. \$\endgroup\$ – Jamal Dec 1 '14 at 15:35
  • \$\begingroup\$ i did jamal, it works fine. but wasn't sure if it correct implementation of <random> or not \$\endgroup\$ – MORTAL Dec 1 '14 at 15:38
  • \$\begingroup\$ This code doesn't appear to compile for me: ideone.com/xb26ZL how have you compiled this? \$\endgroup\$ – shuttle87 Dec 2 '14 at 1:42
  • \$\begingroup\$ @shuttle87 ... i think you may need to provide extra white space at the end of each template members.like this > >::type;. i updated the code. thank you. \$\endgroup\$ – MORTAL Dec 2 '14 at 2:13
  • \$\begingroup\$ The compiler can handle that template syntax fine, the error is this: error: shadows template parm ‘class T’ template<typename T> \$\endgroup\$ – shuttle87 Dec 2 '14 at 2:17
4
\$\begingroup\$

I have a few comments on this.

Don't attempt to nest templates with the same parameter

First, as you've already discovered, nested templates with the same parameter are a violation of the C++ language standard, whether or not your compiler complains. (In fact, I would say it's broken if it does not complain.)

C++ standard, section 14.6.1:

A template-parameter shall not be redeclared within its scope (including nested scopes). A template-parameter shall not have the same name as the template name.

The definition of mUniformDistribution can be simpler

The code doesn't need a using statement. The declaration can be made directly:

decltype(dist<T>()) mUniformDistribution;

(Also note that the existing code misspells "Uniform")

The use of Random does not require a scope

The code in main refers to ::Random but that's really not necessary here. It should just be Random.

Consider repeatability

To answer the question in your code, yes,

std::default_random_engine mEngine{ std::random_device()() };

that line of code does seed mEngine however, I'd suggest rethinking that feature. If you were using this random number generator for, say a simulation, you might want to be able to duplicate the simulation exactly and not simply always run it with new random numbers. To get the best of both worlds, you could keep your existing code, but eliminate the initialization of mEngine and add it instead to the constructors:

Random(const T& min, const T& max)
    : mEngine{}, 
      mUniformDistribution(min, max)
{}
Random(const T& min, const T& max, bool )
    : mEngine{ std::random_device()() }, 
      mUniformDistribution(min, max)
{}
// ...
std::default_random_engine mEngine;

Now if you construct using the first version, you'll get the default seed each time. If you use the second constructor (with a dummy bool parameter) it will use a new seed each time.

\$\endgroup\$
  • \$\begingroup\$ I dont understand the use of dist<T>(). Is it a function, and if so why is it able to be called? It has a non-void return type, but was not defined with a body to return a value? Does the use of decltype just take the return type and bypass the body? \$\endgroup\$ – flakes Dec 2 '14 at 14:47
  • \$\begingroup\$ @Calpratt ... this machinsim known as SFINAE. check this link for more detail en.cppreference.com/w/cpp/language/sfinae \$\endgroup\$ – MORTAL Dec 2 '14 at 15:23

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.