Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

A week ago, I have asked how to simplify a form validation. From the answers, the code now is improved. Can anyone please share their opinion on the approach used below and advise me if there is a better way to cut it?

//Create object to hold the different error messages
var errorMessage = new Object();
errorMessage.required   = "This field can not be empty";
errorMessage.email      = "Please enter a valid email address";
errorMessage.number     = "Please only enter numbers in this field";
errorMessage.min        = "This field should be minimum ";
errorMessage.max        = "This field should be maximum ";
errorMessage.date       = "Please use the date format outlined above";

$("#contactForm").submit(function(event) {

   event.preventDefault();
   if (mtdValidate()) {
     $submit = $(this).find('button[id="submit"]');
       $inputs = $(this).find('input, textarea, select, label');
       var posting = $.post($(this).attr('action'), $('#contactForm').serialize());

       posting.done(function(data) {
          $('span.error').remove();

          if (data == "1") {
             $submit.text('Sent. Thank You!');
             $submit.add($inputs).addClass('sent').prop('disabled', true);
          } else {
             $submit.after('<span style="display: inline-block; padding: 15px 5px; color: #bd3d3d">Failed to send the message, please try again later.</span>');
             $submit.text('Try Again');
          }
       });
   }
});

$("#contactForm input, #contactForm textarea").blur(function() {
   console.log("we blur now");

   if ( $(this).hasClass("required") ) {
      if ($.trim($(this).val()) == "") {
         manageErrorMessage( $(this), errorMessage.required);
         isFormValid = false;
      }

   } else if ($(this).hasClass("email")) {
      var emailRegEx = /^[a-zA-Z0-9._-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4}$/;
      var emailIn = $.trim($(this).val());
      if (emailRegEx.test(emailIn) == false) {
         manageErrorMessage( $(this), errorMessage.email);
         isFormValid = false;
      }

   } else if ($(this).hasClass("number")) {
      var numRegEx = /^[0-9]+/;
      var numIn = $.trim($(this).val());
      if (numRegEx.test(numIn) == false) {
         manageErrorMessage( $(this), errorMessage.number);
         isFormValid = false;
      }

   } else if ($(this).hasClass("date")) {
      var dateRegEx = /^(0?[1-9]|[12][0-9]|3[01])[\/\-](0?[1-9]|1[012])[\/\-]\d{4}$/;
      var dateIn = $.trim($(this).val());
      if (dateRegEx.test(dateIn) == false) {
         manageErrorMessage( $(this), errorMessage.date);
         isFormValid = false;
      }
   }
});

function mtdValidate() {

   //Form is treated as being valid until validation fails
   var isFormValid = true;

   //Reset Error Messages on each submit press
   resetErrorMessages();

   //Loop through fields marked as required
   $("#contactForm input.required").each(function() {
      if ($.trim($(this).val()) == "") {
         manageErrorMessage( $(this), errorMessage.required);
         isFormValid = false;
      }
   });

   //Loop through fields marked as email
   $("#contactForm input.email").each(function() {
      var emailRegEx = /^[a-zA-Z0-9._-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4}$/;
      var emailIn = $.trim($(this).val());
      if (emailRegEx.test(emailIn) == false) {
         manageErrorMessage( $(this), errorMessage.email);
         isFormValid = false;
      }
   });

   //Loop through fields marked as number
   $("#contactForm input.number").each(function() {
      var numRegEx = /^[0-9]+/;
      var numIn = $.trim($(this).val());
      if (numRegEx.test(numIn) == false) {
         manageErrorMessage( $(this), errorMessage.number);
         isFormValid = false;
      }
   });

   //Loop through fields marked as date
   $("#contactForm input.date").each(function() {
      var dateRegEx = /^(0?[1-9]|[12][0-9]|3[01])[\/\-](0?[1-9]|1[012])[\/\-]\d{4}$/;
      var dateIn = $.trim($(this).val());
      if (dateRegEx.test(dateIn) == false) {
         manageErrorMessage( $(this), errorMessage.date);
         isFormValid = false;
      }
   });

   //Loop through fields marked with min and max classes
   $("#contactForm input").each(function() {
      if ( $(this).attr("class") ) {

         if( $(this).attr("class").match(/min[0-9]+/) ) {
            var minClass = $(this).attr("class").match(/min[0-9]+/).toString();
            minVal = parseInt(minClass.match(/[0-9]+/));

            if ( $.trim($(this).val().length) < minVal )  {
               manageErrorMinMax($(this), errorMessage.min, minVal);
               isFormValid = false;
            }

         }

         if ( $(this).attr("class").match(/max[0-9]+/) ) {
            var maxClass = $(this).attr("class").match(/max[0-9]+/).toString();
            maxVal = parseInt(maxClass.match(/[0-9]+/));

            if ( $.trim($(this).val().length) > maxVal )  {
               manageErrorMinMax($(this), errorMessage.max, maxVal);
               isFormValid = false;
            }

         }
      }
   });

   return isFormValid;

}

function resetErrorMessages() {
   $("span.error").remove();
   $("input .error").removeClass("error");
}

function manageErrorMessage(that, errorMessageIn) {
   $(that).addClass('error');
   if ( !$(that).next().is("span") ) {
      $(that).after('<span class="error"></span>');
   }
   $(that).next().html(errorMessageIn);
}

function manageErrorMinMax(that, errorMessageIn, value) {
   $(that).addClass('error');
   if ( !$(that).next().is("span") ) {
      $(that).after('<span class="error"></span>');
   }
   $(that).next().html(errorMessageIn + value + " charaters long.");
}
share|improve this question
add comment

2 Answers

  • I'd almost always rather use object literals {} than new Object() for this. Much less verbose and easier to read.

    var errorMessage = {
      required   : "This field can not be empty",
      email      : "Please enter a valid email address",
      number     : "Please only enter numbers in this field",
      min        : "This field should be minimum ",
      max        : "This field should be maximum ",
      date       : "Please use the date format outlined above"
    };
    
  • You use $('#contactForm') a lot. Save it, and use it as context to your other selectors:

    var $form = $('#contactForm');   // for example this will return all     
    var $inputs = $('input', $form); // inputs that are decendants of $form
    

    Also notice how I prefix variables with $ to denote that they contain jQuery objects. Makes it easier to tell the difference between vars.

  • If you have form elements that need to be validated via regexes, have you thought about putting them in the DOM as data attributes? This keeps formatting rules in one place.

    <input type="text" class="number" data-validation="/^[0-9]+/" />
    

    It also means you don't have to do those multiple iterations. Also, if you use $(this) more than once, save it as a var.

    $('input', $form).each(function () {
       var $el = $(this); 
       var regex = new RegEx($el.data('validation'));
       [...]
    });
    
  • You could use a callback in your $.post to save creating the variable;

      $.post($(this).attr('action'), $('#contactForm').serialize(), 
         function (data) {
           $('span.error').remove();
           [...]
         });
    
  • This is daft:

       if (dateRegEx.test(dateIn) == false) {
           manageErrorMessage( $(this), errorMessage.date);
          isFormValid = false;
        }
    

    No need to check against false, it is false.

      if (dateRegEx.test(dateIn)) {
         [...]
      }
    
  • One more.... Thanks to Hydrothermal for making me realise. This statement will return true if data is any state of truthy because the values are coerced.

        if (data == "1") {
    

    For example:

      true == "1" // true
      1 == "1"    // true
    

    It might be that this doesn't matter in this case, but you should always use === to compare values by identity rather than equality.

share|improve this answer
2  
+1, I have start using daft more ;) –  konijn Feb 6 at 20:32
add comment

It's more of a best practices thing than an actual issue, but you should usually use === instead of == when doing comparisons. === is a "strict" comparison, and is a little more predictable in its behavior. This StackOverflow answer is an excellent explanation of why, if you're curious.

share|improve this answer
    
Also there are some dodgey comparisons going on like this: data == "1". Should data be a String or an number? Does it matter? This statement tells me nothing. –  Jivings Feb 6 at 21:12
    
Haha, also (data == "1") is true if data is truthy. –  Jivings Feb 6 at 21:13
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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