3
\$\begingroup\$

In my HTML, I have a large form (id ="myform") that asks users to fill in 5 fields for Date of Birth (DOB), Sex, Weight, Hair Color and Eye Color and then several other fields which don't relate to this question. Each of those 5 category fields start with a question mark in it. When the users enters something, the question mark goes away. So my JS (which you'll see in a few seconds) looks for a question mark. If it's there, user missed the field. If it's not, the user entered something.

Next to each field is code such as this example for Date of Birth (this is set to display:none by default). It's just the word "Required" in red:

<label id='dob-display-error' class='error' for='dob-display'>Required</label>

I am validating each field using this JavaScript:

$('#myform').submit(function(){
    //check to make sure all categories have been guessed 
            var $dobdisplaytext=$('#dob-display').text();
            var $sexdisplaytext=$('#sex-display').text();
            var $weightdisplaytext=$('#weight-display').text();
            var $hairdisplaytext=$('#hair-display').text();
            var $eyedisplaytext=$('#eye-display').text();

              if ( $dobdisplaytext.indexOf('?') !== -1 ) {
                $('#dob-display-error').show();
                $('#dob').addClass('error-border');
                $('html,body').animate({scrollTop:0},0);
                return false;
              } 
              if ( $sexdisplaytext.indexOf('?') !== -1 ) {
                $('#sex-display-error').show();
                $('#sex').addClass('error-border');
                $('html,body').animate({scrollTop:0},0);
                return false;
              } 
              if ( $weightdisplaytext.indexOf('?') !== -1 ) {
                $('#weight-display-error').show();
                $('#weight').addClass('error-border');
                $('html,body').animate({scrollTop:0},0);
                return false;
              } 

              if ( $hairdisplaytext.indexOf('?') !== -1 ) {
                $('#hair-display-error').show();
                $('#hair').addClass('error-border');
                $('html,body').animate({scrollTop:0},0);
                return false;
              } 
              if ( $eyedisplaytext.indexOf('?') !== -1 ) {
                $('#eye-display-error').show();
                $('#eye').addClass('error-border');
                $('html,body').animate({scrollTop:0},0);
                return false;
              } 
});

Quick rundown of what each thing's purpose is:

$('#dob-display-error').show();

This sets the "Required" error to display:block to alert the user there's an issue

$('#dob').addClass('error-border');

This simply adds a class which puts a red border around the dob field, to further visually alert the user to an issue

$('html,body').animate({scrollTop:0},0);

The form is very long and these fields are toward the top. So when the user is at the bottom of the form and clicks submit, if there's an issue, this sends the user up to the top of the page so they can see the visual alerts.

This code works, however, here are a few things I'd like help with:

  1. I am not even close to a JS expert as I'm sure you can all tell, but there has to be a more efficient way to write this, right? Like in a loop of some sort? With the key being that if four fields are missed, I'd like all four fields to show their respective alerts.

  2. I believe this will be answered by being written better and I alluded to it in #1 but just to be clear and give more details. If I remove return false; from each one, ALL missed categories show the required error/border at once (which I prefer) but of course the form then immediately submits because I have nothing stopping it.

The way it is above, if dob was missed, ONLY the dob error displays (even if they missed others). So that's not a good UX. For example, if the user missed the dob AND sex fields, they'd click "Submit" and get an alert about dob and would choose a dob, then scroll all the way to bottom and click "Submit" again and it would send them back up to fill in Sex. Again, not a good UX.

So I know return false; has got to go SOMEWHERE (and likely only once, once the code is written more efficiently), just not sure where.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

One thing that pops up in my mind when I see code containing repetitions of almost exact copies of structure is how can we achieve reusability?

DRY principle is a very powerful thing that helps identifying and fixing code issues, until gets to the point where the other principles are abused.


This can be solved with code like one below.

  1. First we declare fields to iterate on.
  2. Clear validation error indicators set on previous runs.
  3. filter the fields which fail validation. I am not sure what is the '?' that original code relies on, but I think you know better ;)
  4. If there list of filtered fields is empty, we know there are no validation errors and can return from here.
  5. Otherwise animate to the top of the screen, display UI errors for failed fields, and return false.

$('#myform').submit(function() {
  const allFields = [ 'dob', 'sex', 'weight', 'hair', 'eye' ];

  allFields
    .forEach(nameOfField => {
      $(displayError(nameOfField)).hide();
      $(formField(nameOfField)).removeClass('error-border');
    });

  const listOfFieldsFailedValidation =
    allFields
      .filter(field => {
        const fieldText = $('#' + field + '-display').text();
        return fieldText.indexOf('?') !== -1;
      });

  if (listOfFieldsFailedValidation.length <= 0)
    return true;

  $('html,body').animate({scrollTop:0}, 0);

  listOfFieldsFailedValidation
    .forEach(nameOfFieldFailedValidation => {
      $(displayError(nameOfFieldFailedValidation)).show();
      $(formField(nameOfFieldFailedValidation)).addClass('error-border');
    });

  return false;
});

function displayError(field) {
  return '#' + field + '-display-error';
}
function formField(field) {
  return '#' + field;
}

P.S. I assume ECMAScript 6 version of JS is okay.

\$\endgroup\$

Your Answer

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

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