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.

I wrote some code to validate a form, I'm using live validation, as the user types, here is the snippet that I wrote just for the live validation, but I think I'm repeating too much and I'm looking for some advices:

  $('form input').focus(function(){
      var inp = $(this).attr('name');
      switch(inp)
      {
        case 'password2':
          var theIn = $(this).parent().prev().find('input');
          if(!theIn.val() != '')
          {
            theIn.addClass('error');
          }else{
            theIn.removeClass('error');
          }

        var theInp = $(this).parent().prevAll().find('input,select');
        theInp.each(function(){
        if((!$(this).val()) || ($(this).val() == '-1'))
        {
          $(this).addClass('error');
        }else{
          $(this).removeClass('error');
        }
        });

        break;
        default:
        //Find all previous inputs and see if they are OK
        var theInp = $(this).parent().prevAll().find('input,select');
        theInp.each(function(){
        if((!$(this).val()) || ($(this).val() == '-1'))
        {
          $(this).addClass('error');
        }else{
          $(this).removeClass('error');
        }
        });
      }
    });

/*
* Register front on keypress
 */
  $('form input,form select').each(function(){

    //Select <select>
    $(this).change(function(){
      if($(this).val() == '-1')
      {
        $(this).addClass('error');
      }else{
        $(this).removeClass('error');
      }
    });
    $(this).keyup(function(e){
      var type = $(this).attr('name');
      $(this).parent().find('span.preloader').css('visibility','hidden');
      switch(type)
      {
        case 'email':
          var pattern = /^[-a-z0-9~!$%^&*_=+}{\'?]+(\.[-a-z0-9~!$%^&*_=+}{\'?]+)*@([a-z0-9_][-a-z0-9_]*(\.[-a-z0-9_]+)*\.(aero|arpa|biz|com|coop|edu|gov|info|int|mil|museum|name|net|org|pro|travel|mobi|[a-z][a-z])|([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}))(:[0-9]{1,5})?$/i;
          if(!$(this).val().match(pattern))
          {
            $(this).addClass('error');
          }else{
            $(this).removeClass('error');
          }
        break;
        case 'password2':

          if($(this).val() != $(this).parent().prev().find('input').val())
          {
            $(this).addClass('error');
            $(this).parent().prev().find('input').addClass('error');
          }else{
            $(this).removeClass('error');
            $(this).parent().prev().find('input').removeClass('error');           
          }
        break;
        case 'password':
          if($(this).val() == $(this).parent().next().find('input').val())
          {
            $(this).parent().next().find('input').removeClass('error');
            $(this).removeClass('error');
          }else{
            $(this).addClass('error');
          }
          return true;
        break;
        default:          
          if((!$(this).val()) || ($(this).val() == '-1'))
          {
            $(this).addClass('error');
          }else{
            $(this).removeClass('error');
          }       
      }
  });
});
share|improve this question
add comment

1 Answer 1

Overall, I see the following issues with your code

  • It's coupled way to tightly to the markup
  • It's overly complex
  • You call $(this) again and again without ever caching the result
  • It's repetitive

I won't be posting fully revised code, as there's a lot I'd do differently. Instead, I'll try to explain the steps you could take.

Right now, you're relying on the markup never, ever changing. For instance, you're using $(this).parent().prev().find('input') to find the other password field, but if the markup changes just slightly, your code will no longer find the right input. You'd have to change your code each time you change your markup. Obviously, that's not very maintainable.

You're doing a similar thing to find all other inputs/selects in the form - again, this will break if the markup changes.

Instead, do something like

$("form").each(function () {
  var form = $(this),
      fields = form.find("input, select");

  // ... everything here has access to the form and fields variables
});

Or at the very least use $(this).closest("form").find("input, select")

In either case, you greatly reduce the coupling between markup and code.

Also, do not rely an input's name to tell you its intended type. A name can be anything. For instance, you could have a field called "recipient" which should be an email-address, but then you'd have to update your code to look for that.

In the case of email-addresses specifically, HTML5 has a type-attribute value you can use:

<input name="whatever" type="email">

Your other checks seem to be about checking presence of any value - i.e. required fields. Again, HTML5 has an attribute for that:

<input name="whatever" type="email" required>

So now, your code can check the input's type to determine it's intended type, and check for the required attribute to determine if it's required (seems logical, right?)

Depending on the browser, it might actually "help out" should your validation fail. For instance, Chrome respects and enforces the email type, and so requires the user to type an email-address.

Incidentally, it makes more sense to use an empty string "" as the nothing-selected value in the select element. -1 is a value, whereas an empty string is, well, an empty string; no value at all. JavaScript itself considers "" == false but considers -1 == true.

Last up is the password and password repeat. Instead of the complex jQuery selector chain to find the other password field, use HTML5's data-* attributes to say what you should check against. For instance:

<input id="password" type="password" data-match="password_confirmation">

<input id="password_confirmation" type="password" data-match="password">

Now each element can tell you what its counterpart is, regardless of how your markup is put together.

As for structure and repetition: Don't use the switch statement for big blocks of code. It's hard to read, and hard to maintain. Instead, DRY (Don't Repeat Yourself) you code, by breaking it into functions. For instance, you could have functions like checkEmail and checkSelection that you can pass an element to, and they'll report true or false, depending on element's value.

Do the same for the bit of code that adds the error class: Have a function, like, say

function markElement(element, valid) {
  if(valid) {
    $(element).removeClass("error");
  } else {
    $(element).addClass("error");
  }
}

Now you have some more generic building blocks.

Lastly, don't do all the validation logic in the event handlers themselves. The validation logic is the same regardless of how/why something needs to be validated. Instead, write the validation code separately and use the event handlers to call it.

share|improve this answer
    
Very informative answer, thank you. –  Quentin Pradet Feb 11 '13 at 14:26
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.