5
\$\begingroup\$

I don't mess around a lot with templates (or with GTK), but today I wrote something and would like to know how it can be improved, and hopefully clarify a few things in the process.

So, Randolfi() is a function template which returns a random number. A range can be specified as arguments.

It uses features new to C++11, based on these:

(The name "Randolfi" is a joke made by a few colleagues regarding my last name, Ranolfi.)

randolfi.hpp

#ifndef RANDOLFI_HPP
#define RANDOLFI_HPP

#include <random>

template <typename T>
T Randolfi()
{
    std::mt19937 rng;
    rng.seed( std::random_device()() );
    std::uniform_int_distribution<T> dist;
    return dist(rng);
}

template <typename T>
T Randolfi(T range_start, T range_end)
{
    std::mt19937 rng;
    rng.seed( std::random_device()() );
    std::uniform_int_distribution<T> dist(range_start, range_end);
    return dist(rng);
}

#endif //RANDOLFI_HPP

randolfi-test.cpp

#include <iostream>
#include <string>
#include <gtkmm.h>
#include "randolfi.hpp"

using namespace std;

int main()
{
    auto app = Gtk::Application::create();

    uint a_random_number = Randolfi<uint>();

    Gtk::MessageDialog dialog( to_string(a_random_number) );    
    dialog.run();


    return 0;
}

Notes:

  1. Somehow it doesn't feel right to have such a large proportion of redundant lines across the function overloads. I think there might be a template-related trick to help here, but searching for it proved useless since all the results I found (like this one) are concerned about type parameter overloading, which is not my case.

  2. I though about declaring it as Randolfi(T range_start = NULL, T range_end = NULL) and then checking if (range_start != NULL && range_end != NULL), but that does not save me any lines, and also appears to be frowned upon.

  3. As it is, the function can be called without type argument for the second overload, but not for the first one:

    Randolfi(0, 100);  // type parameter ommited for template
    Randolfi();  // Fails - compiler has no clue on know how to instantiate template
    

    I have a feeling this inconsistency is bad practice.

  4. I'm mostly concerned about the template header, but I did include my testing code (randolfi-test.cpp) to this question so maybe someone can let me know if I did something terribly wrong. From my part, I'd like to avoid calling Gtk::Application::create() but that doesn't seem to be possible.

Thanks!

\$\endgroup\$
6
\$\begingroup\$

The constructor of uniform_int_distribution has default arguments:

template<typename T>
explicit uniform_int_distribution(T a = 0, 
                                  T b = std::numeric_limits<T>::max());

(I don't know why a defaults to 0 rather than to the minimum T, but there you go).

You can reduce your duplication by using the same default arguments, and passing them through to this constructor:

template <typename T>
T Randolfi(T range_start = 0,
           T range_end = std::numeric_limits<T>::max())
{
    std::uniform_int_distribution<T> dist(range_start, range_end);
    return dist(rng);
}

Or, we can use perfect forwarding to create the distribution with the same arguments as passed in:

template <typename T, typename... Args>
T Randolfi(Args&&... args)
{
    std::uniform_int_distribution<T> dist(std::forward<Args>(args)...);
    return dist(rng);
}

We'll need to do this if we want to support a variety of distributions, so it's probably the way to go.


Don't recreate a random generator every call. You could make it static, which will reduce that to once per instantiation, but it's better to keep it outside of the function, so you only create and seed it once per process.

In the test program, you should exercise more than one type T, and test with implicit and explicit range. Also, avoid using namespace std;, and prefer tests which don't require a GUI environment (so they can be more easily run on build servers).


Modified code

#include <limits>
#include <random>
#include <utility>

static auto rng = []{
    std::mt19937 rng;
    rng.seed(std::random_device()());
    return rng;
}();

template <typename T,
          template<typename> typename Distribution
                                      = std::uniform_int_distribution,
          typename... Args>
T Randolfi(Args&&... args)
{
    Distribution<T> dist(std::forward<Args>(args)...);
    return dist(rng);
}

#include <iostream>

int main()
{
    std::clog << Randolfi<int>(-100, +100) << '\n'
              << Randolfi<char>('a', 'z') << '\n'
              << Randolfi<double, std::normal_distribution>(100, 15) << '\n'
              << Randolfi<std::size_t>() << std::endl;
}
\$\endgroup\$
  • \$\begingroup\$ I'm not sure of what you meant with "We'll need to [ use perfect forwarding ] if we want to support a variety of distributions". As is, perfect forwarding integers seems curious. \$\endgroup\$ – papagaga Apr 13 '18 at 11:40
  • \$\begingroup\$ your code won't compile btw. Randolfi signatures overlap (ambiguous). \$\endgroup\$ – papagaga Apr 13 '18 at 11:41
  • \$\begingroup\$ Thanks @papagaga - copy/paste error - I've now removed the accidental extra copy of the uniform_int_distribution version. \$\endgroup\$ – Toby Speight Apr 13 '18 at 12:34
  • \$\begingroup\$ As to your comment - not all distributions take the same number of arguments. For example, std::poisson_distribution accepts only one argument, so couldn't be called with 0 and max in the same way. Forwarding a parameter pack is the only way to support the variable number of arguments required. The perfect part of it is probably now a habit - although I got it wrong (writing (Args...) instead of (Args&&...)), and I got no compiler warning for using std::forward with an argument not of universal-reference type - gcc wishlist! \$\endgroup\$ – Toby Speight Apr 13 '18 at 12:39
  • 1
    \$\begingroup\$ A good learning opportunity with the GUI - how about doing something properly graphical, such as a histogram of results - you should see it approaching the theoretical distribution as you draw more and more selections. That would be a good learning exercise. As to why I avoid using namespace std; even within implementation files: in the worst case it can silently change the meaning of programs (I recommend reading the highest-voted answers on that question for a full perspective, and then make up your mind). \$\endgroup\$ – Toby Speight May 23 '18 at 8:17
4
\$\begingroup\$

As it stands right now, every time you need a random number, you're creating a new instance of std::mt19937, then using std::random_device to seed this instance, then (finally) getting a number and returning it.

What you (nearly always) want to do is create one instance of std::mt19937, seed it once, and then re-use the same generator throughout the remainder of the program.

As it stands now, you'd be much better off just using std::random_device to generate the number (without using std::mt19937 at all):

template <typename T>
T Randolfi()
{
    return std::random_device()();
}

...at which point, the code might as well not exist at all, because it's not adding anything to what std::random_device provides out of the box.

\$\endgroup\$
  • \$\begingroup\$ About your last sentence, it was supposed to be a convenience function with type guarantee. At least that makes for an advantage, no? \$\endgroup\$ – Marc.2377 May 23 '18 at 4:45

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.