Random dice class based off D&D

Question

I am learning C++ and I took on this small project to get more familiar with OOP. The goal was to create a Die class that randomly rolls and returns a value based on the sides of the die, the number of dice rolled, and any added modifiers, (ex: +2).

Die.hpp:

#pragma once
#include <random> //std::random_device, std::mt19937, std::uniform_real_distribution
class Die
{
private:
 int m_sides; //Number of sides on dice
 int m_num; //Number of dice to roll
 int m_mod; //Modifiers that add/sub to overall value
 int m_val; //The die's value when rolled
 static const int sc_MIN_VAL = 1; //Static constant for minimum value of a die roll
 static const int sc_MIN_SIDES = 2; //Static constant for minimum sides of a die, 2 sides = coinflip
 static const int sc_MIN_NUM = 1; //Static constant for minimum number of dice, must be >= 1
 inline static std::random_device s_rd; //Static random seed
 inline static std::mt19937 s_engine; //Static random number generator engine (mt19937) seeded with std::random_device
 std::uniform_int_distribution<int> m_dist; //Uniform int distribution for each die instance, based on number of sides
public:
 Die(); //Constructor #0: Default Constructor, no parameters
 Die(int); //Constructor #1: Dice(sides)
 Die(int, int); //Constructor #2: Dice(sides, number of dice)
 Die(int, int, int); //Constructor #3: Delegated Constructor - Dice(sides, number of dice, modifiers)
 int roll(); //Perform a random dice roll with mt19937 engine, return m_val value of roll
 void setSides(int); //Change the number of sides of the die 
 void setNumber(int); //Change the number of dice rolled
 void setModifier(int m) {m_mod = m;} //Change modifier added to the dice roll
 int getSides() const {return m_sides;} //Returns number of sides of the die
 int getNumber() const {return m_num;} //Returns number of dice being rolled at once
 int getModifier() const {return m_mod;} //Returns modifier being added to dice roll value
};

Die.cpp:

#include "Die.hpp"
//Random Number Generator
std::random_device s_rd;
std::mt19937 s_engine(s_rd());
//Constructor #0: Default Constructor
Die::Die() : Die(6, 1, 0) {}
//Constructor #1
Die::Die(int s) : Die(s, 1, 0) {}
//Constructor #2
Die::Die(int s, int n) : Die(s, n, 0) {}
//Constructor #3: Delegated Constructor
Die::Die(int s, int n, int m) 
 : m_sides(s), m_num(n), m_mod(m), m_dist(sc_MIN_VAL, s)
{
 //Exception Handling
 if(m_sides < sc_MIN_SIDES)
 m_sides = sc_MIN_SIDES;
 if(m_num < sc_MIN_NUM)
 m_num = sc_MIN_NUM;
}
//Main method: returns int value of dice roll sums + modifier
int Die::roll()
{
 //reset m_val to hold new roll value
 m_val = 0;
 //Generate value for dice rolls
 for(int i = 0; i < m_num; i++)
 {
 m_val += m_dist(s_engine);
 }
 //Add modifiers to value, then return m_val
 return m_val += m_mod;
}
//Setters
void Die::setSides(int s)
{
 if(s < sc_MIN_SIDES) //if sides was set to an impossible value less than 2
 m_sides = sc_MIN_SIDES; //set to minimum # of sides, 2
 else m_sides = s; //set normally if value is >= 2
 //Update distribution for random num generator, so that it doesn't become stale
 std::uniform_int_distribution<int>::param_type p(sc_MIN_VAL, m_sides);
 m_dist.param(p);
}
void Die::setNumber(int n)
{
 if(n < sc_MIN_NUM) //if number of dice was set to an impossible value less than 1
 m_num = sc_MIN_NUM; //set to minimum # of dice, 1
 else m_num = n; //set normally if value is >= 1
}

Answer 1

Here are some things that may help you improve your code.

Use include guards

There should be an include guard in each .h file. That is, start the file with:

#ifndef DIE_H
#define DIE_H
// file contents go here
#endif // DIE_H

The use of #pragma once is a common extension, but it's not in the standard and thus represents at least a potential portability problem. See SF.8

Use default parameters for constructors

It isn't really necessary to write three different constructors as you've done. Instead, just create a single one with default parameters.

Simplify your abstraction

I would expect a class named Die to model a single die and not multiple dice. Also, the modifier isn't really intrisic to the object, but a thing that is external to the Die. Really, then, the only thing I would expect the Die to contain would be the number of sides. Here's a full working alternative:

#include <algorithm>
#include <iomanip>
#include <iostream>
#include <map>
#include <random>
class Die {
 static std::random_device rd;
 std::mt19937 engine;
 std::uniform_int_distribution<int> dist;
public:
 Die(int sides)
 : engine{rd()}
 , dist{1, std::max(2, sides)}
 {}
 int operator()() {
 return dist(engine);
 }
};
std::random_device Die::rd;
int main() {
 Die d(20);
 std::map<int, int> history; 
 for (int rolls{20'000}; rolls; --rolls) {
 ++history[d()];
 }
 for (auto item : history) {
 std::cout << std::setw(2) << item.first << ": " << item.second << '\n';
 }
}

Provide a full working example

This comment is really more about how to get a good code review rather than about the code itself. If you add a sample main.cpp and show how you're intending to use the code, it makes things a bit simpler for reviewers (and serves as a form of documentation).

• rkmacleod: Thank you for the feedback! I agree with you and user673679 that this needed to be simplified. My apologies for not providing code from my test sample file. I will remember to add that going forth in future posts. I hope you have a great day!

Answer 2

I'd say this is the wrong place to use an "OOP" approach (is there a right one? :p). I think that what the Die class represents is a bit mixed up, and not so useful in a real world scenario. To whit:

class Die
{
private:
 int m_sides; //Number of sides on dice
 int m_num; //Number of dice to roll
 int m_mod; //Modifiers that add/sub to overall value
 int m_val; //The die's value when rolled

Only the m_sides variable is appropriate in a class that claims to be a Die (i.e. a singular object). If we called the class something else, like DiceRoll they might be better.

Note that we don't need to store m_val in the class at all (it's returned from roll(), and we have no way to access it afterwards) - it can be a local variable.

We should also use full names, and not abbreviations (e.g. m_modifier is much clearer than m_mod).

Since these variables have both setters and getters, there is no point in making them private (of course the setter does bounds checking, but see below...). They should be simple public variables. Which makes the class much simpler!

struct DiceRoll
{
 int m_number = 1;
 int m_sides = 6;
 int m_bonus = 0;
};

We don't need the constructors either, since we can just do DiceRoll{ 2, 3, 1 } or whatever.

---

Die::Die(int s, int n, int m) 
 : m_sides(s), m_num(n), m_mod(m), m_dist(sc_MIN_VAL, s)
{
 //Exception Handling
 if(m_sides < sc_MIN_SIDES)
 m_sides = sc_MIN_SIDES;
 if(m_num < sc_MIN_NUM)
 m_num = sc_MIN_NUM;
}

Note that a) we could use the setSides() and setNumber() here, b) silently correcting values like this is a bad idea - it can lead to unexpected side-effects and hard to diagnose behavior later. It would be much better to actually throw an exception or assert that the values are valid, and then clearly document the range of acceptable values.

---

So on to the rolling:

inline static std::random_device s_rd; //Static random seed
inline static std::mt19937 s_engine; //Static random number generator engine (mt19937) seeded with std::random_device

Making these static (and private) is a bad idea. We often want to be able to save and restore the state of an RNG. It's much more flexible to allow the user to pass the engine into the roll() function.

---

Which brings up another point. We don't need a class to call a function, and we only have 4 arguments to pass in. So the "OOP" stuff is pretty much redundant:

int roll_die(std::mt19937& rng, int num_sides)
{
 assert(num_sides >= 1);
 return std::uniform_int_distribution<int>{ 1, num_sides }(rng);
}
int roll_dice(std::mt19937& rng, int num_dice, int num_sides, int bonus = 0)
{
 assert(num_dice >= 0);
 assert(num_sides >= 1);
 auto total = 0;
 for (auto i = 0; i != num_dice; ++i)
 total += roll_die(rng, num_sides);
 
 return total + bonus;
}

(I'm being slightly more flexible with the allowed number of dice and sides here; I've seen 1d1 rolls in various games. There's even no reason to prevent a 0d1 roll, since it would technically work, even if it's a bit weird).

We could of course check for overflow in an upwards direction, but I've not bothered with that here.

If we do want to group the variables for a roll together because we have to pass them around a lot, we could use the DiceRoll struct above and write another function as a shortcut:

int roll_dice(std::mt19937& rng, DiceRoll roll)
{
 return roll_dice(rng, roll.m_number, roll.m_sides, roll.m_bonus);
}

• rkmacleod: Thank you for the great feedback! I see now that I definitely overcomplicated this. I agree OOP is not best suited here since all the variables are public. Is it a good practice to make members public if you would implement both setters and getters? Thank you again for the help. Cheers!