3
\$\begingroup\$

I have the following function, intended to take standard dice notation (XdY+Z) and return a (sort of) random number based on the input. Are there any bugs/bad ideas/optimizable sections I am missing?

function dieRoll(dice) {
    if (/^[\d+]?d\d+[\+|\-]?\d*$/.test(dice) == false) { //Regex validate
        return "Invalid dice notation"; //Return if input invalid
    }

    if(dice[0]=="d") { //If the first character is a d (dY)
        dice = "1"+dice; //Add a 1
    }
    var minus = dice.search(/\-/); //Search for minus sign

    if (minus == -1 && dice.search(/\+/) == -1) { //If no minus sign and no plus sign (XdY)
        dice += '+0'; //Add a +0
    }
    if (minus == -1) { //If no minus sign (XdY+Z)
        var dicesplit = dice.split('+'); //Split for plus sign
        var modifier = dicesplit[1] * 1; //Number to add to total
    } else { //If there is a minus sign (XdY-Z)
        var dicesplit = dice.split('-'); //Split for minus sign
        var modifier = ("-" + dicesplit[1]) * 1; //Number to add to total
    }

    var diesplit = dicesplit[0].split('d'); //Take the first section (XdY) and split for d
    var howmany = diesplit[0] * 1; //Number of dice to roll
    var diesize = diesplit[1] * 1; //How many sides per die
    var total = 0; //Total starts as 0

    for (var i = 0; i < howmany; i++) { //Loop a number of times equal to the number of dice
        total += Math.floor(Math.random() * diesize) + 1; //Random number between 1 and diesize
    }
    total += modifier; //Add the modifier

    return total; //Return the final total
}
\$\endgroup\$
5
\$\begingroup\$

You validate the input string against a regular expression, but then you manually parse it again the hard way. Instead, you should use parentheses within the regular expression to define capturing groups.

I also think that your regular expression is not quite correct. Optional digits X preceding "d" should be \d* or (\d+)?. An optional ±Z should be ([+-]\d+)?.

function dieRoll(dieSpec) {
    var match = /^(\d+)?d(\d+)([+-]\d+)?$/.exec(dieSpec);
    if (!match) {
        throw "Invalid dice notation: " + dieSpec;
    }

    var howMany = (typeof match[1] == 'undefined') ? 1 : parseInt(match[1]);
    var dieSize = parseInt(match[2]);
    var modifier = (typeof match[3] == 'undefined') ? 0 : parseInt(match[3]);

    …
}
\$\endgroup\$
  • \$\begingroup\$ The two corrections you suggested are valid points; thanks! The group-capturing RegEx seems to work as well. I'm a little confused by the next bit - what exactly are the ? 1 : and ? 0 : parts doing? \$\endgroup\$ – Hydrothermal Feb 5 '14 at 20:09
  • \$\begingroup\$ If omitted, you want the X to default to 1, and Z to default to 0. \$\endgroup\$ – 200_success Feb 5 '14 at 20:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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