HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Random number generation in Dice class

Submitted by: @import:stackexchange-codereview··
0
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 == 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(); }

#endif


Dice.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:

  • 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
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.