Random number generation in Dice class

Question

I don't intend on implementing this into an application yet as I'm going to test it first. As such, it doesn't do much beyond rolling. It also overloads some "typical" operators such as == and !=.

Most of all, I'm still very unsure about my random number generator. I've read about <random>'s std::uniform_int_distribution and that it has advantages over std::rand, but I'm also not sure if I've put it in a good place. Client code will be forced to use my seed if my source file is used, but my header file is free from this.

What could be said about my RNG use? Anything else that could be done?

Dice.h

#ifndef DICE_H
#define DICE_H

#include <ostream>

class Dice
{
private:
    int topValue; // same default type as std::uniform_int_distribution<>

public:
    void roll();
    int value() const { return topValue; }
};

// the C++ FAQ on SO suggests that these overloads are best as non-members

inline bool operator==(Dice const& lhs, Dice const& rhs) { return lhs.value() == rhs.value(); }
inline bool operator!=(Dice const& lhs, Dice const& rhs) { return !operator==(lhs, rhs); }
inline std::ostream& operator<<(std::ostream& out, Dice const& obj) { return out << obj.value(); }

#endif

Dice.cpp

#include "Dice.h"
#include <random>

std::random_device rd;
std::mt19937 gen(rd());
std::uniform_int_distribution<> dis(1, 6);

void Dice::roll()
{
    topValue = dis(gen);
}

Answer 1

Just based on the code, and in no particular order:

• Bikeshedding: as Dice represents a single die, I would call it such.
• You're passing Dice by reference to const everywhere. In your current design it is just an int, and I don't see anything happening that would increase the cost of passing it significantly, or cause it to become non-copyable. As per Optimizing Emergent Structures in C++ you should prefer to pass by value here.
• Your rd, gen, and dis should all be in an anonymous namespace to prevent linker errors when people define things of the same name in other files.
• Does equality and inequality really make sense here? It bothers me that x == y is not preserved after x.roll(); y.roll();. I think I'd stick to accessing value explicitly, and not allow equality on Dice.

As for design: the implicit sharing of state between dice bothers me. It isn't detectable as-is, but it means that there is no way of specifying that a particular die will return particular results (for debugging). If this is used in a multithread application, the results of roll may be surprising; I'm fairly sure uniform_int_distribution<>::operator() should not be called from multiple threads at once.

I think I'd go for the more explicit approach of having an updateRandom and getRandom pair, or ensure thread-safety of roll.

You could also make this more generic:

// die.hpp
extern std::random_device rd;
extern std::mt19937 gen;

template<typename T>
struct Die {
    T value() const {
        return topValue;
    }

    void roll() {
        topValue = dis(gen);
    }

private:
    T topValue;
    static std::uniform_int_distribution<T> dis;
};

template<typename T>
std::uniform_int_distribution<T> Die<T>::dis(1, 6);

// die.cpp
std::random_device rd;
std::mt19937 gen(rd());

However, this would have the obvious drawback of the implementation being exposed.

Answer 2

Some points to add to the others:

• It seems arbitrary to force clients to make two calls for every roll. roll should return the value. You can keep the method to read it, but don't force it's use.
• While I understand the meaning of topValue, I would prefer lastRoll.
• Add numSides to the constructor to allow d20 et al. This value and the type of distribution--uniform, normal, fixed (for testing), etc.--should determine equality. Two dice are equal if they produce similar sequences--not when they happened to roll the same number previously.
• What happens if I access the last roll before rolling the die for the first time? Does this make sense?

Answer 3

Another victim of STL (the guy)'s "rand() Considered Harmful" presentation. I too went and verified that my code was using the random number generators correctly.

Aside from @AntonGolov's advice, I would also mention that Dice::roll is not thread safe. If you have multiple threads rolling dice, then they may cause some weird behaviour.