3
\$\begingroup\$

Is the following code a proper solution to my problem? I have a feeling it can be simplified quite a bit.

If one of the first four checkboxes is checked the sum is 1; if two 1.5; if three 2; if four 2.5. If the bar5 is checked then it's 1 + calculated value of first four (if any is checked). Afterwards, multiply by 100.

function checkboxCount() {
	var form = document.forms["calculator"];
	var bar5 = form.elements["bar5"];
	var foo = document.querySelectorAll("input[name=foo]");
	count = 0;
	
	for (var i = 0; i < foo.length; i++) {
		if (foo[i] && foo[i].checked == true) {
			count++;
		}
	}
	if (count == 1 && bar5.checked == true) {
		count = 1;
	} else if (count == 1) {
		count = 1;
	}
	if (count == 2 && bar5.checked == true) {
		count = 2;
	} else if (count == 2) {
		count = 1.5;
	}
	if (count == 3 && bar5.checked == true) {
		count = 2.5;
	} else if (count == 3) {
		count = 2;
	}
	if (count == 4 && bar5.checked == true) {
		count = 3;
	} else if (count == 4) {
		count = 2.5;
	}
	if (count == 5 && bar5.checked == true) {
		count = 3.5;
	}
	return count;
}

function calculateTotalCost() {
	var totalCost = 0;
	var printCost = document.getElementById('calculatedCost');
	totalCost = 100 * checkboxCount();

	printCost.innerHTML = "$" + totalCost;
}
<form id="calculator">

<fieldset>
<label><input type="checkbox" name="foo" id="bar1" onclick="calculateTotalCost()">Bar 1</label>
<label><input type="checkbox" name="foo" id="bar2" onclick="calculateTotalCost()">Bar 2</label>
<label><input type="checkbox" name="foo" id="bar3" onclick="calculateTotalCost()">Bar 3</label>
<label><input type="checkbox" name="foo" id="bar4" onclick="calculateTotalCost()">Bar 4</label>
<label><input type="checkbox" name="foo" id="bar5" onclick="calculateTotalCost()">Bar 5</label>
</fieldset>

<span id="calculatedCost"></span>
</form>

\$\endgroup\$
3
\$\begingroup\$
  1. The formula can be simplified:

    1. for the first four checkboxes, if any of the items are checked, multiple the number checked by 0.5 and add 0.5
    2. Then add 1 if the fifth checkbox is checked.

    So it might be simpler to just have a reference to the first for checkboxes and then the 5th checkbox. For the first for, one can use document.getElementsByName('foo') to get a NodeList, and then put those nodes in an array using Array.from() and use Array.slice() to get the first four checkboxes.

    var firstFourBars = Array.from(document.getElementsByName('foo')).slice(0, 4);
    

    For the record - I did consider using a CSS selector identical to that of the answer by @200_success (i.e. input[name=foo]:not(#bar5):checked) but because the NodeList returned by document.querySelectorAll() is not live, that DOM query would need to be run each time. See point #3 for more details.

  2. Event delegation can be used to avoid adding a click handler to each checkbox in the HTML. The code below utilizes document.addEventListener() to call the calculateTotalCost whenever a click occurs. Because of this change, that function receives a reference to the event as the first argument. This allows changes based on any click anywhere but conditional logic dependent on event.target could be added.

  3. DOM references can be stored in variables at the start of the script, so as to avoid multiple lookups (e.g. var printCost)
  4. One other simplification:

    var totalCost = 0;
    var printCost = document.getElementById('calculatedCost');
    totalCost = 100 * checkboxCount();
    

    There is little point in setting totalCost to 0 and then assigning it 100 * checkboxCount(). Just assign it the latter product:

    var totalCost = 100 * checkboxCount();
    

document.addEventListener('DOMContentLoaded', function() { //Wait for DOM to be ready
  //cache DOM references
  var printCost = document.getElementById('calculatedCost');
  var firstFourBars = Array.from(document.getElementsByName('foo')).slice(0, 4);
  var bar5 = document.forms["calculator"].elements["bar5"];

  document.body.addEventListener('click', calculateTotalCost);
  // helper function for filter
  function isChecked(checkbox) {
    return checkbox.checked;
  }

  function checkboxCount() {
    var count = firstFourBars.filter(isChecked).length;
    if (count) {
      count = count * 0.5 + 0.5;
    }
    if (bar5.checked) {
      count += 1;
    }
    return count;
  }

  function calculateTotalCost(event) {
    //could conditionally execute based on event.target
    var totalCost = 100 * checkboxCount();
    printCost.innerHTML = "$" + totalCost;
  }

});
<form id="calculator">

  <fieldset>
    <label><input type="checkbox" name="foo" id="bar1">Bar 1</label>
    <label><input type="checkbox" name="foo" id="bar2">Bar 2</label>
    <label><input type="checkbox" name="foo" id="bar3">Bar 3</label>
    <label><input type="checkbox" name="foo" id="bar4">Bar 4</label>
    <label><input type="checkbox" name="foo" id="bar5">Bar 5</label>
  </fieldset>

  <span id="calculatedCost"></span>
</form>

Edit

I forgot to mention that in the modified code above, it waits for the DOM to be ready before before querying it (notice the first line document.addEventListener('DOMContentLoaded', function() { //Wait for DOM to be ready). Not only does it prevent attempts to access DOM elements before they exist, but the variables are scoped to the callback function instead of the global namespace.

\$\endgroup\$
  • \$\begingroup\$ It might make sense to call the function calculateTotalCost directly for the first time. \$\endgroup\$ – sineemore Aug 14 '18 at 20:13
  • \$\begingroup\$ Yeah - maybe... though the OPs code doesn't function that way... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Aug 14 '18 at 20:14
  • \$\begingroup\$ Hey @SamOnela, I just noticed a bug. If you click on Bar 5 first, the value remains $0 instead of $100. Could you please help me? I would greatly appreciate it. \$\endgroup\$ – Tzar Aug 17 '18 at 4:32
  • 1
    \$\begingroup\$ Ah yes- I have updated the code in my answer to handle that. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Aug 17 '18 at 5:55
  • \$\begingroup\$ It works beautifully now. Thank you so much! \$\endgroup\$ – Tzar Aug 17 '18 at 12:25
2
\$\begingroup\$

You aren't taking advantage of the capabilities of document.querySelectorAll(). Furthermore, checkboxCount() would have much simpler logic if you interpreted the requirements more literally: you should count the first four checkboxes separately from the fifth. Finally, I would complain that checkboxCount() is inappropriately named, since it has logic that is much more complicated than counting.

The querySelectorAll() function takes a CSS selector. Not only can the selector specify input[name=foo], but you can also exclude a specific element using :not() and filter by the :checked status.

Instead of if-else cases to determine the "sum", I'd use an array lookup.

Instead of writing an onclick handler for each input element, it would be better practice to eliminate all traces of JavaScript from the HTML, and attach the handlers using JavaScript.

function count(selector) {
    return document.querySelectorAll(selector).length;
}

function calculateTotalCost() {
    var totalCost = 100 * (
        [0, 1, 1.5, 2, 2.5][count("input[name=foo]:not(#bar5):checked")] +
        count("#bar5:checked")
    );
    document.getElementById('calculatedCost').innerHTML = "$" + totalCost;
}

document.querySelectorAll("input[name=foo]").forEach(function(el) {
    el.addEventListener('click', calculateTotalCost);
});
<form id="calculator">

<fieldset>
<label><input type="checkbox" name="foo" id="bar1">Bar 1</label>
<label><input type="checkbox" name="foo" id="bar2">Bar 2</label>
<label><input type="checkbox" name="foo" id="bar3">Bar 3</label>
<label><input type="checkbox" name="foo" id="bar4">Bar 4</label>
<label><input type="checkbox" name="foo" id="bar5">Bar 5</label>
</fieldset>

<span id="calculatedCost"></span>
</form>

\$\endgroup\$
  • \$\begingroup\$ Thank you for such a comprehensive answer. I really appreciate it. Although your code is more concise, Sam’s solution fits better in my form, which is much larger than the code I posted. \$\endgroup\$ – Tzar Aug 14 '18 at 22:54
  • \$\begingroup\$ It's not a good idea to post Code Review questions that are simplified versions of your real code, because then we can't give you the most appropriate advice. \$\endgroup\$ – 200_success Aug 14 '18 at 23:02
  • \$\begingroup\$ I did not know that. It makes sense. Thank you. \$\endgroup\$ – Tzar Aug 14 '18 at 23:10
  • \$\begingroup\$ I posted my full code if you’d like to take a look: Simplifying and Improving Form Calculator \$\endgroup\$ – Tzar Aug 19 '18 at 4:47

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.