3
\$\begingroup\$

In my requestForm submit function I have this logic that checks three checkboxes and then displays the label with an error message. Is there better logic? I would be happy for any kind of feedback since this is quite critical a part of my application.

$("#requestForm").submit(function (e) {
            e.stopPropagation();
            e.preventDefault();


        var checked_sourcecheckboxes = $("#sources input[type=checkbox]:checked");
        if (checked_sourcecheckboxes.length == 0) {
            $("#lblSourcesError").show();
            return false;
        }
        else {
            $("#lblSourcesError").hide();
        }
        var checked_academicformatcheckboxes = $("#academicFormatsRow input[type=checkbox]:checked");
        if (checked_academicformatcheckboxes.length == 0) {
            $("#lblacademicFormatError").show();
            return false;
        }
        else {
            $("#lblacademicFormatError").hide();
        }
        var checked_academicmediaformatcheckboxes = $("#academicMediaFormatRow input[type=checkbox]:checked");
        if (checked_academicmediaformatcheckboxes.length == 0) {
            $("#lblacademicmediaFormatError").show();
            return false;
        }
        else {
            $("#lblacademicmediaFormatError").hide();
        }
})
\$\endgroup\$
2
  • 1
    \$\begingroup\$ what about the return false; logic? that will stop the loop \$\endgroup\$ – Jefferson Feb 9 at 16:08
  • 1
    \$\begingroup\$ You also should post the HTML. Because I would suggest some change... It seems you only use id and you could have a way simpler logic if you'd use classes on the checkboxes and error messages. \$\endgroup\$ – Louys Patrice Bessette Feb 9 at 18:39
3
+25
\$\begingroup\$

Main Question

Is there better logic?

Repeated code

The first thing that I see with this code, which perhaps you see also, is that it is quite repetitive - select a list of checkboxes to see if any are checked - if none are checked then show an error message and return, otherwise hide any affiliated error message. This violates the Don't Repeat Yourself principle. It can be remedied by abstracting the common logic - e.g. putting the the selectors into a mapping:

var SELECTOR_ERROR_MAPPING = {
    "#sources input[type=checkbox]:checked": "#lblSourcesError", 
    "#academicFormatsRow input[type=checkbox]:checked": "lblacademicFormatError",
    //...more entries
};
        

Then use a loop to iterate over the mapping, checking each selector - e.g. using Array.every():

return Object.keys(SELECTOR_ERROR_MAPPING).every(function(selector) {
    var errorSelector = SELECTOR_ERROR_MAPPING[selector];

    var showError = !$(selector).length;
    $(errorSelector).toggle(showError, errorSelector);
    return !showError;
});

Those last three lines of the inner function could be placed into a separate function if desired, which would allow for better modularization/atomization and testing.

demo

var SELECTOR_ERROR_MAPPING = {
  "#sources input[type=checkbox]:checked": "#lblSourcesError",
  "#academicFormatsRow input[type=checkbox]:checked": "#lblacademicFormatError",
  "#academicMediaFormatRow input[type=checkbox]:checked": "#lblacademicmediaFormatError",
  //...more entries
};
$("#requestForm").submit(function(e) {
  e.stopPropagation();
  e.preventDefault();

  var allValid = Object.keys(SELECTOR_ERROR_MAPPING).every(function(selector) {
    var errorSelector = SELECTOR_ERROR_MAPPING[selector];

    var showError = !$(selector).length;
    $(errorSelector).toggle(showError, errorSelector);
    return !showError;
  });
  console.log('result', allValid);
  return allValid;
})
.error {
  color: red;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<form id="requestForm">
  <div id="sources">
    <h3> Source </h3>
    <label>Audi 
      <input type="checkbox" name="source" value="Audi" />
    </label>
    <label>BMW 
      <input type="checkbox" name="source" value="BMW" />
    </label>
    <div class="error" id="lblSourcesError" style="display: none">Please select a source</div>
  </div>
  <div id="academicFormatsRow">
    <h3> Formats </h3>
    <label>College 
      <input type="checkbox" name="source" value="college" />
    </label>
    <label>University
      <input type="checkbox" name="source" value="university" />
    </label>
    <div class="error" id="lblacademicFormatError" style="display: none">Please select a format</div>

  </div>
  <div id="academicMediaFormatRow">
    <h3> Media Formats </h3>
    <label>DVD 
      <input type="checkbox" name="source" value="DVD" />
    </label>
    <label>Blue-ray
      <input type="checkbox" name="source" value="blueray" />
    </label>
    <div class="error" id="lblacademicmediaFormatError" style="display: none">Please select a media format</div>

  </div>
  <button>Submit</button>
</form>

Modern browsers support client-side validation using the required attribute on input elements1 and it is supported for checkbox inputs2 but apparently setting a custom message for checkboxes still involves writing some JavaScript3.

HTML layout

As one comment suggested, the dependence on the id attribute could be decreased - For example, if the location of the error message element was consistent within each section -e.g. a sibling of the list of checkbox inputs. Another solution might be to have each section of checkboxes and error message contained by a parent element with a consistent class name- then the JavaScript could loop over those container elements looking to see if the checkbox elements have any checked- if not, then display the error message.

Other review points

ES6

With modern JavaScript, consider using newer EcmaScript-2015 (AKA ES6) features, like for...of loops, the keywords let and const instead of var, etc.

jQuery

Also consider whether you really need jQuery because it may be the case it can be replaced with “vanilla JS” features. For example, to select elements with a CSS selector use document.querySelectorAll() and to add an event listener for the form submission, use the addEventListener() method after selecting that form element - e.g. with document.getElementById('requestForm'), document.forms[0] unless it isn't the first form, etc..

Loose comparisons

In the original code the length property of the jQuery objects was compared with 0 using loose equality - i.e. ==. A good habit and recommendation of many style guides is to use strict equality operators (i.e. ===, !==) whenever possible. The problem with loose comparisons is that it has so many weird rules one would need to memorize in order to be confident in its proper usage.

The suggested code above and below simplified this by using !$(checkboxSelector).length;

demo

const SELECTOR_ERROR_MAPPING = {
  "#sources input[type=checkbox]:checked": "#lblSourcesError",
  "#academicFormatsRow input[type=checkbox]:checked": "#lblacademicFormatError",
  "#academicMediaFormatRow input[type=checkbox]:checked": "#lblacademicmediaFormatError",
  //...more entries
};
document.forms[0].addEventListener('submit', e => {
  e.stopPropagation();
  e.preventDefault();

  for (const [selector, errorSelector] of Object.entries(SELECTOR_ERROR_MAPPING)) {
    const showError = !document.querySelectorAll(selector).length;
    document.querySelector(errorSelector).classList.toggle('hidden', !showError);
    if (showError) {
      return false;
    }
  }
});
.error {
  color: red;
}
.hidden {
  display: none;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<form id="requestForm">
  <div id="sources">
    <h3> Source </h3>
    <label>Audi 
      <input type="checkbox" name="source" value="Audi" />
    </label>
    <label>BMW 
      <input type="checkbox" name="source" value="BMW" />
    </label>
    <div class="error hidden" id="lblSourcesError">Please select a source</div>
  </div>
  <div id="academicFormatsRow">
    <h3> Formats </h3>
    <label>College 
      <input type="checkbox" name="source" value="college" />
    </label>
    <label>University
      <input type="checkbox" name="source" value="university" />
    </label>
    <div class="error hidden" id="lblacademicFormatError">Please select a format</div>

  </div>
  <div id="academicMediaFormatRow">
    <h3> Media Formats </h3>
    <label>DVD 
      <input type="checkbox" name="source" value="DVD" />
    </label>
    <label>Blue-ray
      <input type="checkbox" name="source" value="blueray" />
    </label>
    <div class="error hidden" id="lblacademicmediaFormatError">Please select a media format</div>

  </div>
  <button>Submit</button>
</form>

\$\endgroup\$

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.