Is this construct a proper use of a private class, and how do I put its instances in that map?

Question

Trying to find a somewhat clean design for sampling billions of dice rolls and while my first ugly solution did work by just creating the generators for the known ranges (dice) it was an ugly mess of multiple typed out by hand copies only changing the range, and switching which to answer the common roll() method with.

#include <random>
#include <map>


namespace dnd {

    class Dice {
    private:
        
        class RandomNumberGenerator {
        public:
            RandomNumberGenerator(int from, int to) {
                distr = std::uniform_int_distribution(from, to);
            }

        private:
            std::random_device rand_dev;
            std::mt19937 generator = std::mt19937();
            std::uniform_int_distribution<int> distr;
        public:
            int roll() {
                return distr(generator);
            }
        };

        static std::map<int, RandomNumberGenerator> generators;

    public:

        static int dN(int n) {
            if (!generators.contains(n)) {
                generators[n] = RandomNumberGenerator(1, n);
            }
            return generators[n].roll();
        }
    };

Is this construct a proper use of a private class, and how do I put its instances in that map? I suppose something is already wrong with the constructor, despite MSVS not complaining? Not sure if even the basic idea is sound (in C++).

I am trying to move from only creating some generators and create them at program start to create every one I need, as using

    public:
        template<typename T>
        static T random(T range_from, T range_to) {
            std::random_device                  rand_dev;
            std::mt19937                        generator(rand_dev());
            std::uniform_int_distribution<T>    distr(range_from, range_to);
            return distr(generator);
        }

        static int d4() {
            static std::random_device                  rand_dev;
            static std::mt19937                        generator(rand_dev());
            static std::uniform_int_distribution<int>    distr(1, 4);
            return distr(generator);
        }
``` ... d20.

is way to slow for the dice not prepared and ugly code wise preparing the expected by hand.

Answer 1

std::random_device can't be copied, therefore instances of your class can't be copied. Bright side, there doesn't seem to be any reason to have a std::random_device member, since you don't use it, and even if you did (to seed the mt19937), you don't need to keep it after it's been used. Just change:

std::random_device rand_dev;
std::mt19937 generator = std::mt19937();  // Should have been std::mt19937(rand_dev()) to actually seed it at all

to:

// Seeds it with a temporary instance of random_device
std::mt19937 generator = std::mt19937(std::random_device{}());

to both create and initialize the generator without preserving a std::random_device that never gets used again (it serves no purpose after that point).

You also need to avoid the use of generators[n] (which implicitly requires a default constructor for RandomNumberGenerator, which it does not have), and make some other small cleanups (e.g. using initialization of distr instead of reassigning it after default initialization, and I had to move the definition of generators inside dN's definition or you get linker errors for reasons I'm not 100% clear on), but once that is done, it works just fine:

#include <random>
#include <map>
#include <iostream>

namespace dnd {

    class Dice {
    private:
        
        class RandomNumberGenerator {
        public:
            RandomNumberGenerator(int from, int to) : distr(from, to) {}

        private:
            std::mt19937 generator = std::mt19937(std::random_device{}());
            std::uniform_int_distribution<int> distr;
        public:
            int roll() {
                return distr(generator);
            }
        };

    public:

        static int dN(int n) {
            static std::map<int, RandomNumberGenerator> generators;
            if (!generators.contains(n)) {
                generators.emplace(n, RandomNumberGenerator(1, n));
            }
            return generators.find(n)->second.roll();
        }
    };
}

int main() {
   for (int i = 0; i < 10; ++i) {
      std::cout << dnd::Dice::dN(12) << std::endl;
   }
}

Attempt This Online!

Answer 2

You are copying the object, but your copy constructor is deleted, per the error message:

main.cpp:32:31: error: object of type 'mapped_type' (aka 'dnd::Dice::RandomNumberGenerator') cannot be assigned because its copy assignment operator is implicitly deleted
                generators[n] = RandomNumberGenerator(1, n);
                              ^
main.cpp:17:32: note: copy assignment operator of 'RandomNumberGenerator' is implicitly deleted because field 'rand_dev' has a deleted copy assignment operator
            std::random_device rand_dev;
                               ^
...

You can find more information about copy constructors here: https://en.cppreference.com/w/cpp/language/copy_constructor

Also look at this answer: What is The Rule of Three?