current community

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite

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;
    int add = toInt(dicePart3);
    roll = toInt(dicePart1) * (rand() % toInt(dicePart2)) + add;
    printf("roll = [%i]\n",roll);

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

    return 0;
}
share|improve this question
1  
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 13 hours ago
add comment

2 Answers

up vote 9 down vote accepted

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.

share|improve this answer
add comment
up vote 6 down vote

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 '+'.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.