Calculate sum of values from checkboxes

Question

I have a bunch of checkboxes with a data-amount attribute containing a value (positive or negative).

My goal is to generate a running total as the user checks each box, and then output this later on.

My code works - just curious for feedback as using filter, map and reduce seems overkill for something like this.

Thanks

const checkboxes = document.querySelectorAll('input[type=checkbox]');      
const output = document.querySelector('.runningTotal');

checkboxes.forEach(function(checkbox) {
        checkbox.addEventListener('change', function() {
            const runningTotal =
                Array.from(checkboxes) 
                .filter(i => i.checked) // remove unchecked checkboxes.
                .map(i => i.dataset.amount ??= 0) //extract the amount, or 0
                .reduce((total, item) => { return total + parseFloat(item)}, 0)
                
            console.log(runningTotal)
            output.innerHTML = runningTotal;
            
        })
    });

<input type="checkbox" data-amount="100"> 100
<input type="checkbox" data-amount="150"> 150
<input type="checkbox" data-amount="-50">  -50
<input type="checkbox" data-amount="10.50"> 10.50
<input type="checkbox" data-amount="0"> 0

<div class="runningTotal"></div>

Answer 1

Your code is too complex.

General points

• Use textContent when setting text in an HTMLElement rather than innerHTML which forces a reflow.

• Use a single listener rather than one for each clickable element. See rewrite.

• The nullish assignment is not required. i => i.dataset.amount ??= 0 is more efficient as i => i.dataset.amount ?? 0 as it does not modify the markup if amount is undefined.

  However I can not see why you vet the dataset values.

• Better to use Number to parse a string to a number.

• Prefer id to uniquely identify DOM elements.

• Don't add comment that state the obvious. Generally good code is self documenting (meaningful naming, structured, etc...) and thus does not need comments

• Don't leave console output in release code.

• Use the spread operator ... to convert from iterable array like objects to array. Eg Array.from(checkboxes) is the same as [...checkboxes]

Rewrite

The rewrite adds a span to accept a single click event that is used to calculate the sum.

The listener only iterates the checkboxes once per click to calculate the sum.

The element used to display the sum is identified by its id rather than a className

The code assumes that all the data set values are correctly setup and thus only need to check if the checkbox is checked.

const checkboxes = document.querySelectorAll("input[type=checkbox]");      
sumCheckboxes.addEventListener('click', () => {
    var total = 0;
    for (const {checked, dataset} of checkboxes) {
        total += checked ? Number(dataset.amount) : 0;
    }
    runningTotal.textContent = total;
});

<span id="sumCheckboxes">
<input type="checkbox" data-amount="100"> 100
<input type="checkbox" data-amount="150"> 150
<input type="checkbox" data-amount="-50">  -50
<input type="checkbox" data-amount="10.50"> 10.50
<input type="checkbox" data-amount="0"> 0
</span>
<div id="runningTotal"></div>

Alternative implementation using a Array.reduce to sum the checkboxes. Note that the array like result of querySelectorAll needs to be converted to an array. This is done once using the spread operator.

const checkboxes = [...document.querySelectorAll("input[type=checkbox]")];  
sumCheckboxes.addEventListener('click', () => {
    runningTotal.textContent = checkboxes.reduce((total, el) =>
        total + (el.checked ? Number(el.dataset.amount) : 0),
        0
    )
});

<span id="sumCheckboxes">
<input type="checkbox" data-amount="100"> 100
<input type="checkbox" data-amount="150"> 150
<input type="checkbox" data-amount="-50">  -50
<input type="checkbox" data-amount="10.50"> 10.50
<input type="checkbox" data-amount="0"> 0
</span>
<div id="runningTotal"></div>

Answer 2

Your code is pretty small, and pretty easy to follow.
You seem to have tried to make everything concise and avoid really long lines that go on forever and ever and ever and ever and ever and ever and ever after...

However, there are a few things that do need to be improved, and where I disagree with the existing answer.

But first...

Lets address the existing answer

I have absolutely no ill intent against that answer. It is a good answer, but it needs improvements as well.

• Use textContent not because it does a reflow, but because it prevents security issues. In this case, it doesn't matter much, but keep this in mind.
  Using innerHTML will render any HTML you send, as well as text, which can be BAD.

• Use a single listener rather than one for each clickable element.
  As in the presented rewrite, you can just put an event listener on the top most common element.

• The nullish assignment is not required. Using it, in this case, is a bad idea. You should just use the value attribute.
  The value attribute is always available in any <input> element.
  If you're worried about the checkboxes being submitted, you don't need to worry: they don't have names and won't be sent in a form submit.

• To uniquely identify a single element, using id is the best.
  However, you can use a class to identify all the elements you need.
  It isn't a requirement, but, it may work best in the future.

• Instead of using the spread operator or something to convert the NodeList into an array, just loop over the NodeList.
  Just use forEach with an early return for those elements you aren't interested in.

My rewrite

Personally, I would rewrite this in a much simpler way:

const checkboxes_holder = document.getElementById('checkboxes');
const checkboxes = checkboxes_holder.querySelectorAll('input[type="checkbox"]');
const output = document.getElementById('total');

checkboxes_holder.addEventListener('change', function(){
  var total = 0;
  checkboxes.forEach(function(checkbox){
    if(!checkbox.checked) return;
    
    total += parseFloat(checkbox.value) || 0;
  });
  output.innerText = total;
});

<fieldset id="checkboxes">
  <input type="checkbox" value="100"> 100
  <input type="checkbox" value="150"> 150
  <input type="checkbox" value="-50">  -50
  <input type="checkbox" value="10.50"> 10.50
  <input type="checkbox" value="0"> 0
</fieldset>
<output id="total">0</output>

Comments on my rewrite

Yes, there's stuff to say about it...

• The use of a <fieldset> instead of a <div> has different presentation, but has a better semantic meaning than a <span> or <div>.
  Same for the <output> tag, which has a better semantic meaning.
  You can change it at your pleasure, without any changed to the JavaScript.

• I know it is against conventions to use names_with_underscores, but when reading, it's a lot easier to distinguish between methods and variables if they are written differently.
  The use of camelCase for variables makes them look too similar to methods.

• All checkboxes could be created in JavaScript, instead of creating them in HTML, for extra flexibility.

• This code is running "just because".
  Please wrap it in an window.addEventListener('load', [...]);.
  Or use a defer attribute, if it is loaded remotely, from an URL.
  Or make sure the <script> tag is at the end of the document.

• I should use square brackets on the if. However, it looks better without them...

• The .reduce() method, provided by @Blindman67, looks a lot more slick and simpler, but requires converting to an array.
  I avoided that conversion deliberately, but it is a perfectly valid option.

• parseFloat(checkbox.value) || 0 <-- this isn't the cleanest, but avoids issues where something fails to parse.
  When parseFloat or the Number constructor or +'string' fail to convert into a number, it will return NaN.
  If you add NaN, everything becomes NaN.
  Using ... || 0 returns 0 in cases where you would get an NaN.
  However, it is very ugly, and should be handled in a much more verbose way, with Number.isNaN (preferred option) or isNaN.

As a last point...

There's a lot of leeway on a simple script like this, but, hope I've presented a good enough alternative.

I'm open to discussing and changing my answer according to new details and/or more context.