3
\$\begingroup\$

How could I make this code dry?

$("#btnSubmitToCheckOut").click(function(event) {
        var isSelected = [];

        $(".age_agree :checkbox").each(function() {
            if ($(this).is(":checked")) {
                isSelected.push("true");
            } else {
                isSelected.push("false");
            }
        });

        if ($.inArray("false", isSelected) < 0) {
            $(this).parents('form').submit();
        } else {
            alert('Please accept all the Terms and Conditions');
        }

    });
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

I was thinking something along these lines. The condition checks whether there are no unchecked checkboxes (length of the selection is 0) and then executes the appropriate branch.

$("#btnSubmitToCheckOut").click(function(event) {
    if ($('.age_agree input[type="checkbox"]').not(':checked').length == 0) {
        $(this).parents('form').submit();
    } else {
        alert('Please accept all the Terms and Conditions');
    }
});
\$\endgroup\$
7
  • \$\begingroup\$ I think I'd make the length > 0 explicit in the condition, as it is easier to read on first sight. \$\endgroup\$
    – Nihathrael
    Nov 20, 2014 at 10:39
  • \$\begingroup\$ @Nihathrael I was waiting for that comment. I guess it's a matter of preference, but since this is code review, and clarity never hurts, I'll update my answer. \$\endgroup\$ Nov 20, 2014 at 10:40
  • 1
    \$\begingroup\$ Please, use .age_agree input[type="checkbox"] instead. You will notice a difference in performance on most modern browsers. :checked is a jQuery extension and will be processed by javascript, while input[type="checkbox"] will be processed by querySelectorAll() (if supported) instead, which will be parsed using the CSS selector engine of the browser. \$\endgroup\$ Nov 21, 2014 at 0:18
  • \$\begingroup\$ @IsmaelMiguel Thanks for the insights. I've tested this with 10 checkboxes in a simple page in Chrome, and over 1 million executions, your version is indeed about 4.5 times as fast (35 seconds vs 165 seconds). I've updated my answer. \$\endgroup\$ Nov 21, 2014 at 1:14
  • \$\begingroup\$ Forgot one important thing! The documentation backing up what I stated: api.jquery.com/checkbox-selector (you should add this to your answer) \$\endgroup\$ Nov 21, 2014 at 1:24
1
\$\begingroup\$

I came up with this somewhat shorter solution.

Using an array to find out if one of the checkboxes has not been clicked is unnecessary here, as you are only interested in if all checkboxes are clicked or not. That leads to two possible solutions, one is using a booling to saving the state using a normal for loop or using an early return to break from the loop in case a checkbox that has not been clicked is found. I decided to go with the early return as it shortens the method and removes the need for the if/else construct at the end.

$("#btnSubmitToCheckOut").click(function(event) {
    var checkBoxes = $(".age_agree :checkbox")

    for(int i=0; i < checkBoxes.length; i++) {
        if (!checkBoxes[i].is(":checked")) {
           alert('Please accept all the Terms and Conditions');
           return;
        }
    };

    $(this).parents('form').submit();
});

What do you think?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.