1
\$\begingroup\$

I have a bunch of checkboxes that need to be toggled based on certain conditions. My function is called by multiple buttons on an onclick event. The certain condition is expressed as a string (check_all, uncheck_all, check_all_virtual). The problem that I see is that if the function would be called as toggleCheckboxes('typo'), that the error wouldn't be so easy to see as there would be no error message. Such problems should probably yield more information. How could this be done better?

<div>
    <input name="site_ids[]" data-is-virtual="0" value="1" class="checkbox"/> SG3
    <input name="site_ids[]" data-is-virtual="1" value="2" class="checkbox"/> SG1
    <input name="site_ids[]" data-is-virtual="0" value="3" class="checkbox"/> SG2
</div>
<div>
    <button onclick="toggleCheckboxes('check_all')">Check All</button>
    <button onclick="toggleCheckboxes('uncheck_all')">Uncheck All</button>
    <button onclick="toggleCheckboxes('check_all_virtual')">Check All Virtual</button>
</div>

<script type="text/javascript">
var checkboxes = document.getElementsByClassName('checkbox');

function toggleCheckboxes(operation) {
    for (var i = 0, n = checkboxes.length; i < n; i++) {
        switch (operation) {
            case 'check_all':
                checkboxes[i].checked = true;
                break;
            case 'uncheck_all':
                checkboxes[i].checked = false;
                break;
            case 'check_all_virtual':
                if (checkboxes[i].getAttribute('data-is-virtual') == true) {
                    checkboxes[i].checked = true;
                }
                break;
        }
    }
}
</script>

The functionality is correct. Checking all virtual shouldn't uncheck non-virtual.

\$\endgroup\$
1
  • \$\begingroup\$ Couldn't for(var i = 0, n = checkboxes.length; i < n; i++) be just for(var i = 0; i < checkboxes.length; i++)? The only reason I'd use the former, would be if I was aiming optimization (in case the checkboxes.length method was being called every iteration on the second code), but apparently on modern browsers both will only call it once if you don't change the length during the loop \$\endgroup\$ – IanC Nov 15 '16 at 13:28
2
\$\begingroup\$

I may have misunderstood something but if you just want to be able to detect case where the input string is none of expected, you may do something like that:

default:
  console.log("This operation '"+operation+"' is not supported, it will be ignored");

at the end of your switch\case construction

\$\endgroup\$
0
\$\begingroup\$

Not sure what you want to achieve on this code, anyway I see the following improving points.

In this case, your code have what I think is a bug.

If you check all and then you check virual, I would expect that only the virtual checkbox will be checked, and other no.

In your code nothing will change.

Than you access this attribute directly from DOM getAttribute() call, instead you should access by dataset, as this kind of attributes are special in HTML5.

As the data-is-virtual value is a string you shouldn't compare with a boolean value, as you assign integers (0|1).

I suggest to change the code like so:

checkboxes[i].checked = (+checkboxes[i].dataset.isVirtual == 1);

The + force to convert the string to an integer value.

In this way only the data-is-virtual will be checked, and the others will be unchecked as expected.

But about this, I'm not sure what you want, I just say what I will expect to happen if I use this code.

The use of the switch helps the readability but if you want to cut your code, you could do like so:

if (operation == 'check_all_virtual') {
    checkboxes[i].checked = new Boolean(checkboxes[i].dataset.isVirtual);
} else {
    checkboxes[i].checked = (operation == 'check_all');
}

Another point of improvement is to use the arrays functions instead of a for loop:

function toggleCheckboxes(operation) {
    if (operation == 'check_all_virtual') {
        checkboxes.forEach(function (checkElement) {
            if (operation == 'check_all_virtual') {
               checkElement.checked = new Boolean(checkElement.dataset.isVirtual);
            } else {
               checkElement.checked = (operation == 'check_all');
            }
        });
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Hi. I've updated my question. Please have another look. String to boolean comparison - I'm using just the equal value operator (==) and not equal value & type (===). new Boolean(checkboxes[i].dataset.isVirtual) wouldn't be ok because '0' would always equal to true. An int conversion is missing here. variable == true keeps it simple enough in my opinion. Why would using the arrays function be an improvement compared to a for loop? Please elaborate. \$\endgroup\$ – ifsession Nov 15 '16 at 12:29
  • \$\begingroup\$ @ifsession yes, I changed this part according with the exact value of the data-if-virtual. Please have a look on it. \$\endgroup\$ – Mario Santini Nov 15 '16 at 12:50

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.