-1

The function is the following:

float random_float(float min, float max)
{
    std::random_device rd;                                            // obtain a random number from hardware
    std::mt19937 gen(rd());                                           // seed the generator
    std::uniform_real_distribution<> distr((double)min, (double)max); // define the range
    return (float)distr(gen);
}

I would like to avoid calling the first 3 lines every time I call this function. I also would rather not create a class with a constructor, then having to instantiate it every time I just want to generate a random number. I'm not very familiar with what is possible with modern C++ features, so I would like some ideas.

mbl
  • 805
  • 11
  • 18
  • 3
    You most definitely should "avoid calling the first 3 lines". These first three lines should be called, at most, only once. Unfortunately, if you ask ten C++ developers "what is the simplest and most elegant way to do ", you will get eleven different answers. This is purely a matter of opinion, and something that you need to decide by yourself. – Sam Varshavchik Oct 22 '20 at 22:41
  • 1
    Not exactly shiny and new but a static class or namespace could work. [Namespace + functions versus static methods on a class](https://stackoverflow.com/q/1434937) – 001 Oct 22 '20 at 22:44
  • This really depends on what you want to do. There are arguments for using function static variables, making those statics `thread_local` or making a whole class of it, or just keeping the components separate. Each approach has its merits. Your current approach, though, is probably always wrong for reasons of efficiency. – Galik Oct 22 '20 at 23:00
  • Also note: [Seeding `mt19937` is hard](https://stackoverflow.com/q/45069219/364696). As written, you'll only seed a tiny fraction of the possible RNG state (of course, this hardly matters if you're seeding from system randomness every time and always generating numbers in a narrow range, though the former is kind of a bad idea). – ShadowRanger Oct 22 '20 at 23:04
  • There is a header-only library that seeks to provide the various options in an easy to use and efficient way: https://gitlab.com/Galik/randomly-useful – Galik Oct 22 '20 at 23:22
  • Look up Lambda Expressions. Very handy. I use lambdas to wrap random number generation so I can get something that looks a lot like `d6()` to roll a six-sided die. – user4581301 Oct 22 '20 at 23:30

2 Answers2

7

Your function is seeding a new random number generator every time it's called. You should generally only seed one once (per thread) since seeding a PRNG takes time and it usually serves no purpose re-seeding it.

A minimal rewrite:

#include <random>
#include <type_traits>

auto& generator() { // reuse for ALL random_float-like functions
    // make it thread_local if you have more than one thread using it:
    static std::mt19937 gen(std::random_device{}());
    return gen;
}

template<typename T>
std::enable_if_t<std::is_floating_point_v<T>, T> 
random_float(T min, T max)
{
    std::uniform_real_distribution<T> distr(min, max);
    return distr(generator());
}

template<typename T>
std::enable_if_t<std::is_integral_v<T>, T> 
random_int(T min, T max)
{
    std::uniform_int_distribution<T> distr(min, max);
    return distr(generator());
}

To only instantiate one distribution object (per required distribution) you could add:

template<float min, float max> // C++20 required (and doesn't work in clang yet)
auto random_float_with_static_distribution() {
    // this also needs to be thread_local if you plan on using it in multiple threads:
    static std::uniform_real_distribution<float> distr(min, max);
    return distr(generator());
}
Ted Lyngmo
  • 93,841
  • 5
  • 60
  • 108
3

As always the answer is: it depends.

It depends on the architecture of your program. One good way would be to have an object responsible for generating sequences of random numbers and having references to it through it your program. A quick dirty trick to make it easily available is to make it a singleton.

A simpler, easier solution is to make the variables static inside the function:

float random_float(float min, float max)
{
    static std::random_device rd{};
    static std::mt19937 gen{rd()};

    std::uniform_real_distribution<float> distr{min, max};
    return distr(gen);
}

The uniform_real_distribution is a light object so recreating it every time shouldn't be a problem.

Also generating a double and then converting it to float makes no sense so I took the liberty to change that part of your code.

bolov
  • 72,283
  • 15
  • 145
  • 224
  • 1
    "*The `uniform_real_distribution` is a light object so recreating it every time shouldn't be a problem*" - more importantly, since the `min` and `max` values are caller-supplied values, they could be different on multiple calls to `random_float()`, so `distr` can't be `static` anyway. – Remy Lebeau Oct 23 '20 at 00:04
  • @RemyLebeau you could make it static and then call `params` to set the min/max. I think this will preserve the internal state of the distribution, potentially saving some random calls over time. But I don't think it's worth the hassle. – bolov Oct 23 '20 at 11:08
  • that would introduce a race condition if this code ever needed to be called across multiple threads. – Remy Lebeau Oct 23 '20 at 14:28
  • @RemyLebeau true – bolov Oct 23 '20 at 17:15