patterncppMinor
Random number generation in Dice class
Viewed 0 times
randomnumbergenerationdiceclass
Problem
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
Most of all, I'm still very unsure about my random number generator. I've read about `
What could be said about my RNG use? Anything else that could be done?
Dice.h
Dice.cpp
== and !=.Most of all, I'm still very unsure about my random number generator. I've read about `
'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
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(); }
#endifDice.cpp
#include "Dice.h"
#include
std::random_device rd;
std::mt19937 gen(rd());
std::uniform_int_distribution<> dis(1, 6);
void Dice::roll()
{
topValue = dis(gen);
}Solution
Just based on the code, and in no particular order:
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
I think I'd go for the more explicit approach of having an
You could also make this more generic:
However, this would have the obvious drawback of the implementation being exposed.
- Bikeshedding: as
Dicerepresents a single die, I would call it such.
- You're passing
Diceby reference to const everywhere. In your current design it is just anint, 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, anddisshould 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 == yis not preserved afterx.roll(); y.roll();. I think I'd stick to accessingvalueexplicitly, and not allow equality onDice.
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
struct Die {
T value() const {
return topValue;
}
void roll() {
topValue = dis(gen);
}
private:
T topValue;
static std::uniform_int_distribution dis;
};
template
std::uniform_int_distribution Die::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.
Code Snippets
// 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());Context
StackExchange Code Review Q#31571, answer score: 6
Revisions (0)
No revisions yet.