D&D dice rolling app

This JavaScript object is part of a mobile application that rolls dice for our D&D games.

Here are some example of possible inputs from the user:

• 2d4

• 2d4 + 3d6

• 2d4 + 3d6 + 12

function DiceRoller() {
    this.rollDices = function(input){

        var parsedInput = input.split("+");
        var totalScore = 0;

        for (var diceIndex = 0; diceIndex  0) {
                var diceParts = diceRoll.split('d');

                //We need a random roll for each dice. ex: 4d6, needs 4 rolls.
                for (var numberOfDiceIndex = 0; numberOfDiceIndex < diceParts[0];numberOfDiceIndex++) {
                    totalScore += getRandomInt(1,diceParts[1]);
                }
            } else {
                totalScore += parseInt(diceRoll);
            }
        }

        return totalScore;
    };

    var getRandomInt = function(min, max){
        return Math.floor(Math.random()*(max-min+1)+min);
    }
}

I'm concerned about the performance of the rollDices method, considering I use split quite a lot and I have nested loops.

Performance here won't be a problem. The splits you have are relatively trivial, and presumably there is a human component in here too (someone entering the roll-specs), so the parse time will essentially be negligible compared to that.

The code itself though has some neat features, and also some concerns.

Random

First up, the getRandomInt method is good. It follows the classic definition for retrieving a random value from a specified closed range [low,high]. I would prefer for there to be spaces on the operators though. Your code:

return Math.floor(Math.random()*(max-min+1)+min);

should be:

return Math.floor(Math.random() * (max - min + 1) + min);

Numbers

Your code makes some lucky guesses about numbers, and although it works, it is also easy to break. For example, if I take your random function again:

return Math.floor(Math.random()*(max-min+1)+min);

and instead make it:

return Math.floor(Math.random()*(1+max-min)+min);

it should work fine, right? Except, it won't, because you give the max value to the function as string:

var diceParts = diceRoll.split('d');

.....
    totalScore += getRandomInt(1,diceParts[1]);
.....

So, since you pass in diceParts[1] as a string, the 1+max-min will become 1 + "4" - 1 which in turn becomes 14 - 1 instead of 5 - 1.... so, your code works, but is it an accident? Well, not really, because you figured this out in the other side of the situation - where there is no d in the spec. For that, you have:

totalScore += parseInt(diceRoll);

You should be more defensive about number-handling in your code. You will run into mysterious bugs at some time in the future if you don't control them now.

parseInt(...)

You should always give parseInt a radix to work with. The documentation says:


Specify 10 for the decimal numeral system commonly used by humans. Always specify this parameter to eliminate reader confusion and to guarantee predictable behavior. Different implementations produce different results when a radix is not specified.

(note: the bold emphasis is from the documentation, it is not my addition)

Since parseInt() ignores leading whitespace, and stops parsing on the first non-digit, it is OK to give parseInt(...) padded values - you don't need to trim them first.

Map->Reduce

I would have liked to have seen a map-reduce process happening here:

• you have a + separated sequence of inputs

• each part has a roll/limit

• each result should be summed.

I would be tempted to build the process as:

this.rollDices = function(spec) {
    return spec.split(/\+/)
          .map(parseRoll)
          .reduce(function(a,b){return a + b;});
};

The above code describes the process quite well:

• split on the +

• convert parts to values

• sum each part.

The implementation details for parseRoll is:

var getRandomInt = function(min, max){
    return Math.floor(Math.random() * (max - min + 1) + min);
};

var parseValue = function(val) {
    return parseInt(val || "1", 10);
}

var parseRoll = function(roll) {
    var parts = roll.split(/d/);
    var sum = 0;
    var limit = parseValue(parts[1]);
    for (var i = parseValue(parts[0]) - 1; i >= 0; i--) {
        var got = getRandomInt(1, limit);
        sum += got;
        // console.log("From roll " + roll + " part " + i + " got " + got + " sum " + sum);
    }
    return sum;
};

I have put the above in to a snippet here:



var getRandomInt = function(min, max){
return Math.floor(Math.random() * (max - min + 1) + min);
};

var parseValue = function(val) {
return parseInt(val || "1");
}

var parseRoll = function(roll) {

// only here for the snippet
debug("Parsing " + roll);

var parts = roll.split(/d/);
var sum = 0;
var limit = parseValue(parts[1]);
for (var i = parseValue(parts[0]) - 1; i >= 0; i--) {
var got = getRandomInt(1, limit);
sum += got;

// only here for the snippet
debug(" From roll " + roll + " part " + i + " got " + got + " sum " + sum);

}
return sum;
};

var rollDices = function(spec) {
// only here for the snippet
debugarea.value = "";

return spec.replace(/[^+0-9d]+/g, "")
.split(/\+/)
.map(parseRoll)
.reduce(function(a,b){return a + b;});
};

// *
// only here for the snippet
// *

var results = document.getElementById("result");

var debugarea = document.getElementById("debug");

var debug = function(txt) {
debugarea.value = debugarea.value + txt + "\n";
}

function updated(input) {
results.value = rollDices(input.value);
}
#spec {
background-color: lightgreen;
}

#result, #debug: {
background-color: lightgray;
}
Dice Spec :

Dice Value:

Debug:



StackExchange Code Review Q#86548, answer score: 13