# Simple dice roll using std::string as dice notation

How this could be optimized (if there any need for optimization) and what are your thoughts?

This will be a function that returns a random number from Dice Notation (more on Wikipedia). I will later use this function to determine whether some skill check is passed or not.

#include <string>

using namespace std;

int toInt(string text) {
return atoi(text.c_str());
}

int main(int argc, char* argv[]) {

srand(time(NULL));

string diceString = "1d6+4";
unsigned int dice1number = 0;

string info = "dice ["+diceString+"] = %i\n";
printf("==[START]================\n");

printf(info.c_str(),dice1number);

// parse dice
unsigned int i=0;
unsigned int part = 1;

string dicePart1 = "";
string dicePart2 = "";
string dicePart3 = "";

for(i=0;i<diceString.length();i++) {

if (diceString[i]=='d') {
part = 2;
continue;
}

if (diceString[i]=='+') {
part = 3;
continue;
}

if (diceString[i]=='-') {
dicePart3 += "-";
part = 3;
continue;
}

if (part==1 && diceString[i]>='0' && diceString[i]<='9') {
dicePart1 += diceString[i];
}

if (part==2 && diceString[i]>='0' && diceString[i]<='9') {
dicePart2 += diceString[i];
}

if (part==3 && diceString[i]>='0' && diceString[i]<='9') {
dicePart3 += diceString[i];
}

}

printf("dicePart1 = [%s]\n",dicePart1.c_str());
printf("dicePart2 = [%s]\n",dicePart2.c_str());
printf("dicePart3 = [%s]\n",dicePart3.c_str());

int roll = 0;
roll = toInt(dicePart1) * (rand() % toInt(dicePart2)) + add;
printf("roll = [%i]\n",roll);

printf("==[END]==================\n");

return 0;
}

-
Hi, and welome to CodeReview's 'First Question' review. It would be helpful if you added a bit more of a description about what your code is supposed to do. Adding a description gives the reviewer something to reference back to, and to identify discrepancies. Otherwise, decent question! –  rolfl Feb 11 at 14:49

Your code has an integer-division/multiplication problem/bug.

Consider the dice-roll specification 10d10+0.

Your code will parse this down to:

string dicePart1 = "10";
string dicePart2 = "10";
string dicePart3 = "0";


and this will be used as:

int add = toInt(dicePart3);
roll = toInt(dicePart1) * (rand() % toInt(dicePart2)) + add


At face value, this may be OK, but, in reality, the calculation boils down to:

roll = 10 * (rand() % 10) + 0;


This will only ever be able to produce 10 results, 0, 10, 20, 30, ...., 90, which really is not what you want.

You really should do this as a loop, or as a floating point operation.... the loop seems to be easier:

roll = toInt(dicePart3);
for (i = toInt(dicePart1); i > 0; i--) {
roll += (rand() % toInt(dicePart2));
}


Edit: Second bug

In addition to the bug I pointed out above, I have actually identified a second bug. Dice are always 1-based.... you cannot roll a '0'. So, the method would actually have to be:

roll = toInt(dicePart3);
for (i = toInt(dicePart1); i > 0; i--) {
roll += 1 + (rand() % toInt(dicePart2));
}


Edit: discussion on distribution....

you may be tempted to be more efficient and use the function:

int p1 = toInt(dicePart1);
int p2 = toInt(dicePart2);
int p3 = toInt(dicePart3);

roll = p1 + (rand() % (p1 * p2)) + p3;


The above will create a result that is distributed in the correct range.... but the actual distribution curve is flat.... the odds of a 10d10+0 being 10 is the same as it being 50.... but, in reality, there are many ways to throw a 50, but only 1 way to throw a 10. As a result, the roll-value of 10d10+0 is not evenly distributed in the range 10 ... 100. You need to use a method where each dice is rolled individually.

int p1 = toInt(dicePart1);
int p2 = toInt(dicePart2);
int p3 = toInt(dicePart3);

....

roll = p3;
for (i = p1; i > 0; i--) {
roll += 1 + (rand() % p2);
}


The above will, for 10d10+0 produce values in the correct range 10 ... 100 and the value frequencies will be distributed normally, as expected.

-

I would recommend a more object-oriented approach.

I would encapsulate the functionality of the dice into a class.
Here's an example of what the class could look like:

class Dice
{
public:
Dice () ;
Dice (const std::string &diceNotation) ;
int Roll () const ;

private:
int n1 ;
int n2 ;
int n3 ;

friend std::istream& operator>> (std::istream& is, Dice &dice) ;
friend std::ostream& operator<< (std::ostream& os, const Dice &dice) ;
};


I would use operator>> and operator<< for inputting and outputting data respectively. Please note, this is just to give you an idea. I did minimal testing with this.

std::istream& operator>> (std::istream& is, Dice &dice)
{
is >> dice.n1 ;
is.ignore () ;
is >> dice.n2 ;
is >> dice.n3 ;

return is ;
}

std::ostream& operator<< (std::ostream& os, const Dice &dice)
{
std::string sign = (dice.n3 > 0) ? "+" : "" ;
os << dice.n1 << "d" << dice.n2 << sign << dice.n3 ;
return os ;
}


Then you could use operator>> in your constructor.

Dice::Dice () : n1 (0), n2 (0), n3 (0)
{
}

Dice::Dice (const std::string &diceNotation) : n1 (0), n2 (0), n3 (0)
{
std::stringstream ss (diceNotation) ;
ss >> *this ;
}


Here's a main() function just to show how this could be used.

int main ()
{
Dice d1 ("1d6+4") ;
Dice d2 ("3d8-5") ;
Dice d3 ("345d12+55");

std::cout
<< d1 << "\n"
<< d2 << "\n"
<< d3 << "\n" ;

return 0;
}


One thing I will note, is that streams can be pretty slow on some systems.
An alternate way to parse the strings would be to use std::string::find() to find the index of the 'd' and std::string::find_first_of() to find the index of the '-' or '+'.

-