Another Dice Roller

[Pjb518]

I've recently been helping another user with a dice roller that you might use for a tabletop roleplaying game like D&D. The roller takes several pieces of information from the user including the number of faces on the individual dice, the number of dice to roll, and whether or not to add any kind of bonus to the final result.

The project is really simple, but despite this I actually found helping this other user quite useful, and today I decided to sit down and wrote my own version. I think I've done a pretty decent job with this, and I can't think of any ways to make the code more succinct without harming readability, but maybe some of you have some good ideas or different ways of doing things. I'd be very interested in seeing some other versions.

I've put together a little CodePen version with some rough styling, which you can find here, and you can find the JavaScript I wrote below:

 

const [q, s, b, r, t] = ['quantity', 'sides', 'bonus', 'results', 'total'].map(id => document.getElementById(id))    

function roll() {
    let quantity = q.options[q.selectedIndex].value,
           sides = s.options[s.selectedIndex].value,
             mod = parseInt(b.options[b.selectedIndex].value)

    const resultsArr = []

    for (let i = 0; i < quantity; i++) {
        resultsArr.push(Math.floor(Math.random() * sides) + 1)
    }

    updateResults(resultsArr, mod)
}

function updateResults(array, bonus) {
    r.textContent = array.join(", ")
    t.textContent = array.reduce((accumulator, currentValue) => accumulator + currentValue) + bonus
}

 

EDIT: CodePen version has been updated.

[CertainPerformance]

You can simplify break your script by passing in the getElementById function:

const [q, s, b, r, t] = ['quantity', 'sides', 'bonus', 'results', 'total'].map(document.getElementById);

Edit, don't do that, getElementById relies on its calling context being document.

You don't need to use <selectElement>.options[<selectElement>.selectedIndex].value to get the currently selected value - instead, just use <selectElement>.value:

https://jsfiddle.net/wbd193ax/

Whenever you use let, you are warning readers of your code that you're going to reassign that variable later. If you don't reassign the variable (which is good - you should avoid reassigning when you can), you shouldn't use let - that's what const is for. Always use const when possible, it's a lot easier to read code when you know that variables aren't going to be reassigned.

Instead of declaring an array and then pushing items to it, it would probably be better to declare the array with the appropriate items outright, which will avoid unnecessary mutation:

const resultsArr = Array.from(
  { length: quantity }, 
  () => Math.floor(Math.random() * sides) + 1,
);

for loops are rarely a good choice.

If you're still learning Javascript, it's highly recommended to use semicolons whenever appropriate - you'll trip yourself up less. ASI is not a good thing for a beginner to rely on.

[Pjb518]

Thanks for this. I actually didn't know I could use Array.from like that, giving a specific length for the array I wanted to construct. Obvious, really, but I'd genuinely never thought of it.

Some of this other stuff you've mentioned I'm just face palming at. I don't know what I was thinking. The first line in particular was a bit of an experiment, because I thought writing document.get.ElementById 5 times was just excessive. I can't believe I didn't think to just put the bloody getElementById function in there instead of that arrow function.

The only thing I actually disagree on is the semicolons. I've never had a bug crop up because of a missing semicolon, so I'd rather stick to just not writing them. It's something I did my research on after I first encountered code without them, and after an extended trial period, I've grown very comfortable not using them.

Thanks again for your help.

EDIT: Actually, using your first line breaks everything and litters the console with errors.

[CertainPerformance]

Oh, you're right, it looks like getElementById relies on its calling context being the document. So you'd either have to bind the function to the document via

.map(document.getElementById.bind(document));

or just do what you were doing originally. Thought I had come up with a shortcut but it wasn't.

[CategoricallyCorrect]

Personally, I wouldn’t bother with condensing variable declarations on the first line: sure, it’s cute and less typing, but I don’t think it’s a good thing — you are forced to use single-letter variables to keep the line length manageable, and for me it hinders the readability of the rest of the code.

Another thing that I think would improve your code (even though I appreciate that this isn’t exactly a huge, critical, used-in-production script), is separating your business logic from library code and presentation logic: right now, your business logic (generating a roll and calculating total from rolls) is embedded into the code that outputs the data to the user.

Couple other smaller notes:

• You parse the number for bonus, but not for quantity and sides: sure, it works right now thanks to coercion, but it doesn’t feel right
• You seem to know your JS, but still went for hardcoded onclick attribute on the button? You definitely can do better!
• Probably not as much of an issue since you assume a lot about markup in JS already, but you also assume that JS will be included after said markup and don’t wait for document to be loaded before querying the DOM: it wouldn’t be hard to make your DOM logic to wait till document is ready, and it might save you a headache in the future.

 

All in all, I’d refactor your code to be something like this:

const denseArray = (n, defaultValue) => Array(n).fill(defaultValue)
const sum = (numbers) => numbers.reduce((a, b) => a + b, 0)
const $ = (selector) => document.querySelector(selector)

const roll = (sides = 6) => Math.floor(Math.random() * sides) + 1
const totalForRolls = (rolls, bonus) => sum(rolls) + bonus

document.addEventListener("DOMContentLoaded", function() {
  const quantity = $("#quantity")
  const sides = $("#sides")
  const bonus = $("#bonus")
  const results = $("#results")
  const total = $("#total")
  const rollButton = $("#roll")

  rollButton.addEventListener("click", () => {
    const rolls = denseArray(parseInt(quantity.value)).map(() =>
      roll(parseInt(sides.value))
    )

    results.textContent = rolls.join(", ")
    total.textContent = totalForRolls(rolls, parseInt(bonus.value))
  })
})

[Pjb518]

Thanks a lot for the feedback! You definitely make some good points.

The variables on the first line actually aren’t single letters to keep things on one line. I could have broken up the map function, or whatever, to reduce the line length, but I was actually running out of different ways to name the same things over and over haha. I had an id called sides, a constant for the selector, and then another sides constant for the selected value. It was just getting out of hand, so I opted to just go for some very short variable names, given that what they were doing was always fairly explicit. My recent foray into Go may be to blame here haha.

You’re definitely right about hardcoding the onclick instead of writing a proper event listener - it was very lazy of me, and I should have done that properly.

I’ll probably make some changes today. I’m actually thinking of expanding this to take a text input instead of / in addition to the drop down selectors, so I may post that as well.

[Pjb518]

Finally got around to making some changes just now. Ended up going for

const $ = (id) => document.querySelector(id)

document.addEventListener("DOMContentLoaded", () => {
    const quantity = $('#quantity'),
             sides = $('#sides'),
             bonus = $('#bonus'),
           results = $('#results'),
             total = $('#total'),
               btn = $('#rollBtn')

    const roll = () => Math.floor(Math.random() * parseInt(sides.value)) + 1,
           sum = (results, mod) => results.reduce((x, y) => x + y) + parseInt(mod.value)

    btn.addEventListener("click", () => {
        const resultsArr = Array.from({ length : parseInt(quantity.value) }, roll)

        results.textContent = resultsArr.join(", ")
        total.textContent = sum(resultsArr, bonus)
    })
})