current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 1 down vote favorite

Would like some assistance in improving my jQuery validation code. I have some repeated code, but I am not sure how to write it in a way so that I don't have to keep repeating the same code over and over again for different variable names etc... (such as the if statement part) My goal is to make it so that I can easily apply it to any other form I create.

Here is the jsFiddle

(function($) {

    $('.commentForm').submit(function () {

        var error = 'error';

        var name = $(this).find('.name')
        nameVal = name.attr('value')
        email = $(this).find('.email')
        comment = $(this).find('textarea')
        commentVal = comment.html();

        if (!(name.val() === nameVal || name.val() === '' || name.val().length < 3) && 
            !(comment.val() === commentVal || comment.val() === '' || comment.val().length < 3) && 
            validateEmail(email.val())
           ) {
            console.log('Form is good');
            return true;
        } else {

            (name.val() === nameVal || name.val() === '' || name.val().length < 3) ? name.addClass(error) : name.removeClass(error);
            (comment.val() === commentVal || comment.val() === '' || comment.val().length < 3) ? comment.addClass(error) : comment.removeClass(error);
            (!validateEmail(email.val())) ? email.addClass(error) : email.removeClass(error);

            console.log('Form is BAD');
            return false;

        }

    });

    function validateEmail(email) {
        var re = /^(([^<>()[\]\\.,;:\s@\"]+(\.[^<>()[\]\\.,;:\s@\"]+)*)|(\".+\"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/;
        return re.test(email);
    }

}) (jQuery);
share|improve this question
    
If you can use plugin, jquery validation plugin comes with email validation. If not, you can have a look at their implementation. You can find it here: validation.bassistance.de/email-method –  user27584 Jul 23 '13 at 2:51
1  
Likely moot at this point, but you can remove the check for $this.val() === '' as it is true if $this.val().length < 3 is true. –  Daniel Cook Nov 20 '13 at 14:24

2 Answers 2

up vote 2 down vote accepted

This confuses me:

name.val() === nameVal

The variable nameVal is equal to the value attribute of name. And, the calling val() on name returns the value attribute of name so of course they are going to be equal.

I may be missing something here, but I think this is unnecessary.

Long chunks of mush like this:

(name.val() === nameVal || name.val() === '' || name.val().length < 3) ? name.addClass(error) : name.removeClass(error);
            (comment.val() === commentVal || comment.val() === '' || comment.val().length < 3) ? comment.addClass(error) : comment.removeClass(error);
            (!validateEmail(email.val())) ? email.addClass(error) : email.removeClass(error);

and this:

!(name.val() === nameVal || name.val() === '' || name.val().length < 3) && 
            !(comment.val() === commentVal || comment.val() === '' || comment.val().length < 3) && 
            validateEmail(email.val())

are in no way whatsoever readable. You need to either

  • Break this up. Maybe everything here is not necessary.

  • Split this into variables and check with the variables.

I think you should go with the first option, because these can be simplified:

1:

As I stated above, having

name.val() === nameVal

is unnecessary. You can just remove this.

2:

The value "" is called a falsey value. That means that "" can be treated as though it were false.

So, for example, this here:

name.val() === ''

can be reduced to:

!name.val()

Constantly calling the .val function on these elements in each of your conditionals is inefficient. You should store the result of .val() in a variable, and just use that for the conditionals (I think nameVal should already hold the .val() for name).

Now, for example, one of your conditional chunks becomes:

( !nameVal || nameVal.length < 3) ? name.addClass(error) : name.removeClass(error);

(! commentVal || commentVal.length < 3) ? comment.addClass(error) : comment.removeClass(error);

(!validateEmail(email.val())) ? email.addClass(error) : email.removeClass(error);

Where commentVal is comment.val() and nameVal is name.val()

share|improve this answer
up vote 0 down vote

You could replace your if block with: 

(name.val() === nameVal || name.val() === '' || name.val().length < 3) ? name.addClass(error) : name.removeClass(error);
(comment.val() === commentVal || comment.val() === '' || comment.val().length < 3) ? comment.addClass(error) : comment.removeClass(error);
(!validateEmail(email.val())) ? email.addClass(error) : email.removeClass(error);

if(name.hasClass(error) || comment.hasClass(error) || email.hasClass(error))
{
    console.log('Form is BAD');
    return false;
}
else
{
    console.log('Form is good');
    return true;
}

This would run your validation logic once and then look for error classes on the objects

share|improve this answer

protected by Ethan Bierlein Jul 5 at 1:58

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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