Loop in a function either skips a logic test or doesn't evaluate properly

Question

The function below is fired when any of a series of 17 checkboxes is checked. The idea is the user can check at most, three of them. Once they do, the unselected checkboxes are meant to disable themselves, preventing others from being checked. If all three are checked and the user unchecks one of those, the disabled checkboxes are supposed to enable again.

The code appears to execute fine on its first loop. totalChecks becomes 1. On the second loop however, it then bypasses the first if statement (totalChecks will still be 1 at this point) and disables all checkboxes for some reason.

When I run the webpage and check any one checkbox, the code adds 1 to the totalChecks variable. However, on the second loop, it seems to think that totalChecks is greater than 3 and then disables all of the checkboxes, including the one I selected even if totalChecks is 1. Is my syntax perhaps not set up properly or am I not using conditional tests right

I'm a little out of my depth. Apologies if it ends up being a simple issue.

The bark() function is just my shorthand of writing "console.log"

function checkMS()
{
//first, sum the amount of checked boxes
bark("\n\n\tSection 1 - INIIALISING FUNCTION!")

var totalChecks = 0 //sets to 0 each time function is used
bark("\t\ttotalChecks initialised at "+totalChecks+".\n")

bark("\tSection 2 - ENTERING FOR LOOP!")
for(var i = 0; i < msCheckboxes.length; i++)
{
    bark("\t\tSection 2.1 - FOR LOOP: checking totalChecks < 3")
    if (totalChecks < 3)
    {
        bark("\t\t\tTotal checks is LESS than 3")
    bark("\t\t\tOn LOOP: "+i+", msCheckboxes["+i+"] is "+msCheckboxes[i].checked+".\n")
        if (msCheckboxes[i].checked == true)
        {
            msCheckboxes[i].disabled = false;
            totalChecks ++;
            bark("\ttotalChecks updated to "+totalChecks+".\n\n")
        }
    }
        else if(totalChecks == 3 && msCheckboxes[i].checked === false )
            bark("All checks used... "+totalChecks+". Disabling others")
        {
            {
                bark("Unchecked checkboxes should be disabled now")
                msCheckboxes[i].disabled = true; //disable unchecked checkboxes at limit

            }
        }
}
}

It is almost impossible to understand what you ask. Please read this [ask], and then provide a [mcve]. — Asons, Jul 07 '18 at 13:19
I'd just use a selector with the [*checked* property](https://developer.mozilla.org/en-US/docs/Web/CSS/:checked). Whenever any is clicked on, count the number checked and if it's 3, disable the rest. If it's less than 3, enable them all. — RobG, Jul 07 '18 at 13:24
BTW, you don't need to test boolean properties with *true*, so `if (msCheckboxes[i].checked)` is sufficient. ;-) — RobG, Jul 07 '18 at 13:26
@RobG - I forgot about [`:checked`](https://www.w3.org/TR/selectors-3/#checked) being CSS! (As opposed to just jQuery.) — T.J. Crowder, Jul 07 '18 at 13:28
Apologies for being unclear. Edited the question now. Thanks for the advice too @RobG — Basil Nagle, Jul 07 '18 at 13:35

T.J. Crowder · Accepted Answer · 2018-07-07T13:47:07.530

You can't use a single loop for this. First you need to determine whether three checkboxes are checked, and then you need to disable the others.

Here's a simple way to do that, see comments (but there's a simpler one after it using :checked and :not(:checked)):

document.getElementById("container").addEventListener("click", function() {
  // Get the checkboxes as an array
  var cbs = Array.prototype.slice.call(document.querySelectorAll("#container input[type=checkbox]"));
  // Find out how many are checked
  var total_checked = cbs.reduce(function(sum, cb) {
    return sum + (cb.checked ? 1 : 0);
  }, 0);
  var disable = total_checked == 3;
  // Enable/disable others
  cbs.forEach(function(cb) {
    if (!cb.checked) {
      cb.disabled = disable;
    }
  });
});

<div id="container">
  <div><label><input type="checkbox" value="1"> One</label></div>
  <div><label><input type="checkbox" value="2"> Two</label></div>
  <div><label><input type="checkbox" value="3"> Three</label></div>
  <div><label><input type="checkbox" value="4"> Four</label></div>
  <div><label><input type="checkbox" value="5"> Five</label></div>
  <div><label><input type="checkbox" value="6"> Six</label></div>
  <div><label><input type="checkbox" value="7"> Seven</label></div>
  <div><label><input type="checkbox" value="8"> Eight</label></div>
  <div><label><input type="checkbox" value="9"> Nine</label></div>
  <div><label><input type="checkbox" value="10"> Ten</label></div>
</div>

Or, using the :checked pseudo-class (thanks for the reminder, RobG!):

document.getElementById("container").addEventListener("click", function() {
  // How many checked checkboxes are there?
  var total_checked = document.querySelectorAll("#container input[type=checkbox]:checked").length;
  // Enable/disable the others
  var disable = total_checked == 3;
  document.querySelectorAll("#container input[type=checkbox]:not(:checked)").forEach(function(cb) {
    cb.disabled = disable;
  });
});

<div id="container">
  <div><label><input type="checkbox" value="1"> One</label></div>
  <div><label><input type="checkbox" value="2"> Two</label></div>
  <div><label><input type="checkbox" value="3"> Three</label></div>
  <div><label><input type="checkbox" value="4"> Four</label></div>
  <div><label><input type="checkbox" value="5"> Five</label></div>
  <div><label><input type="checkbox" value="6"> Six</label></div>
  <div><label><input type="checkbox" value="7"> Seven</label></div>
  <div><label><input type="checkbox" value="8"> Eight</label></div>
  <div><label><input type="checkbox" value="9"> Nine</label></div>
  <div><label><input type="checkbox" value="10"> Ten</label></div>
</div>

Note: The NodeList from querySelectorAll only fairly recently got the forEach method, but you can trivially polyfill it; see this answer. Here's the above with the polyfill:

if (typeof NodeList !== "undefined" &&
    NodeList.prototype &&
    !NodeList.prototype.forEach) {
    // Yes, direct assignment is fine here, no need for `Object.defineProperty`
    NodeList.prototype.forEach = Array.prototype.forEach;
}

document.getElementById("container").addEventListener("click", function() {
  // How many checked checkboxes are there?
  var total_checked = document.querySelectorAll("#container input[type=checkbox]:checked").length;
  // Enable/disable the others
  var disable = total_checked == 3;
  document.querySelectorAll("#container input[type=checkbox]:not(:checked)").forEach(function(cb) {
    cb.disabled = disable;
  });
});

<div id="container">
  <div><label><input type="checkbox" value="1"> One</label></div>
  <div><label><input type="checkbox" value="2"> Two</label></div>
  <div><label><input type="checkbox" value="3"> Three</label></div>
  <div><label><input type="checkbox" value="4"> Four</label></div>
  <div><label><input type="checkbox" value="5"> Five</label></div>
  <div><label><input type="checkbox" value="6"> Six</label></div>
  <div><label><input type="checkbox" value="7"> Seven</label></div>
  <div><label><input type="checkbox" value="8"> Eight</label></div>
  <div><label><input type="checkbox" value="9"> Nine</label></div>
  <div><label><input type="checkbox" value="10"> Ten</label></div>
</div>

This is way more effective than I realised. Thanks for the help — Basil Nagle, Jul 07 '18 at 13:52

score 0 · Answer 2 · answered Jul 07 '18 at 13:29

This else if statement is not properly structured bark gets before having curyl parenthesis

function checkMS() {
  var msCheckboxes = [{
    checked: true,
    disabled: true
  }, {
    checked: true,
    disabled: true
  }, {
    checked: true,
    disabled: true
  }, {
    checked: false,
    disabled: true
  }];
  //first, sum the amount of checked boxes
  console.log("\n\n\tSection 1 - INIIALISING FUNCTION!")

  var totalChecks = 0 //sets to 0 each time function is used
  console.log("\t\ttotalChecks initialised at " + totalChecks + ".\n")

  console.log("\tSection 2 - ENTERING FOR LOOP!")
  for (var i = 0; i < msCheckboxes.length; i++) {
    console.log("\t\tSection 2.1 - FOR LOOP: checking totalChecks < 3")
    if (totalChecks < 3) {
      console.log("\t\t\tTotal checks is LESS than 3")
      console.log("\t\t\tOn LOOP: " + i + ", msCheckboxes[" + i + "] is " + msCheckboxes[i].checked + ".\n")
      if (msCheckboxes[i].checked == true) {
        msCheckboxes[i].disabled = false;
        totalChecks++;
        console.log("\ttotalChecks updated to " + totalChecks + ".\n\n")
      }
    } else if (totalChecks == 3 && msCheckboxes[i].checked === false) {
console.log("All checks used... "+totalChecks+". Disabling others")
      console.log("Unchecked checkboxes should be disabled now")
      msCheckboxes[i].disabled = true; //disable unchecked checkboxes at limit

    }
  }
}

checkMS();

score 0 · Answer 3 · answered Jul 07 '18 at 13:39

The reason for your issue is because of the bark function under your else-if statement.

else if(totalChecks == 3 && msCheckboxes[i].checked === false )
            **bark("All checks used... "+totalChecks+". Disabling others")** <---- this line
        {
            {
                bark("Unchecked checkboxes should be disabled now")
                msCheckboxes[i].disabled = true; //disable unchecked checkboxes at limit

            }
        }

The bark function needs to be inside the brackets. Currently, all the code inside the brackets is executing on every iteration of the for loop.

Loop in a function either skips a logic test or doesn't evaluate properly

3 Answers3