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.

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
}
share|improve this question

1 Answer 1

up vote 2 down vote accepted

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]);

    …
}
share|improve this answer
    
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? –  Hydrothermal Feb 5 at 20:09
    
If omitted, you want the X to default to 1, and Z to default to 0. –  200_success Feb 5 at 20:17
    
Thanks; that makes perfect sense! –  Hydrothermal Feb 5 at 20:23

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.