JavaScript Validation

Question

Bit of a preface on my motive: I'm self taught and have never really worked in a collaborative environment. My only benchmark for quality is "does it work" and I've never been really subject to peer review.

With that in mind I just thought I'd dump a small jQuery validation plugin I wrote here and see if anyone can spot any rookie errors or advise on best practice. I'm very aware that because I've never had anyone looking over my shoulder while I'm coding, I could be committing all sorts of programmatic faux pas and not even realising.

Here's the plugin in its entirety followed by some example markup (sorry about the annoying comments)

Demo Link

// Using a global "Tiga" object. This allows me to extend the same object with all my different plugins

     if (typeof Tiga == "undefined") {
            Tiga = {};
        }

        //extend jQuery with the .tigaValidate method. This allows for easy running
        $.fn.tigaValidate = function(options) {
            // Plugin Settings
            var s = $.extend( {
                // Default Settings
                form : this,
                live : true,
                formatting : true,
                invalid : function($form){
                    $("body, html").animate({
                        scrollTop: 0
                    }, 500);
                    $form.addClass("error");
                }
            }, options);

            //let's just call our the init() method of our core object now.
            Tiga.Validate.init(s.form, s.live, s.formatting, s.invalid);
        };


        // here's our core object
        Tiga.Validate = {
            init: function(form, live, formatting, invalid){
                // defaults (for when/if Tiga.Validate.init() is invoked directly)
                if (typeof live == "undefined") { live = true; };
                if (typeof formatting == "undefined") { formatting = true; };
                // cache $(form)
                var $form = $(form),
                    // keycodes to be excluded from the keyup binding ( left & right arrows, backspace, delete, shift and ctrl )
                    allowKeys = [37, 39, 8, 46, 17, 16];
                // disable HTML5 validation
                $form.attr("novalidate", true);
                // if live validation is enabled:
                if (live) {
                    // ..find and cache our submit button before disabling it
                    var $submit = $(form.selector + " .submit");
                    $submit.attr("disabled", true);
                    // ..traverse the DOM and look for required fields, bind validation to the blur event
                    // (all traversal accomplished with sizzle when possible - http://jsperf.com/jquery-find-vs-sizzle/3)
                    $(form.selector + " [required]").on("blur", function(e){ 
                        //validate that mofo
                        Tiga.Validate.validateSingle($(this));
                        //validation check on form and enable/disable the button;
                        if (Tiga.Validate.validateForm(form)) {
                            $submit.attr("disabled", false);
                        } else {
                            $submit.attr("disabled", true);
                        };
                    });
                } else {
                // otherwise bind validation to form submit
                    $form.on("submit", function(e){
                        //validate all forms at once
                        $(form.selector + " [required]").each(function(i,o){
                            Tiga.Validate.validateSingle($(o));
                        });
                        //valid - submit the form
                        if (Tiga.Validate.validateForm(form)) {
                            return;
                        //invalid - scroll to the top and show some error messages maybe.
                        } else {
                            //run invalid callback
                            invalid($form);
                            e.preventDefault();
                            return false;
                        };
                    });
                };
                // if fomatting is enabled lets also check for special, funky field types (like cards) so we can do some on-the-fly formatting.
                if (formatting) {
                    $(form.selector + " input[pattern='card_date'], " + form.selector + " input[pattern='card_number'], " + form.selector + " input[pattern='card_cvv']").on("keyup", function(e){ 
                        var code = e.keyCode || e.which;
                        // check key being pressed is not an allowed key.
                         if(allowKeys.indexOf(code) == -1 ) {
                            if ($(this).is("[pattern='card_date']")){
                                Tiga.Validate.cardFormat(this, 7, "card_date");
                            }
                            if ($(this).is("[pattern='card_number']")){
                                Tiga.Validate.cardFormat(this, 19, "card_number");
                            }
                            if ($(this).is("[pattern='card_cvv']")){
                                // simplified version just automatically strips out letters on keyup
                                Tiga.Validate.cardFormat(this);
                            }
                        }
                    });
                    //postcode capitalisation
                    $(form.selector + " input[pattern='postcode']").on("keyup", function(e){
                        var code = e.keyCode || e.which;
                        // check key being pressed is not an allowed key.
                         if(allowKeys.indexOf(code) == -1 ) {
                            var val = $(this).val().toUpperCase();          
                            $(this).val(val);
                        }
                    });
                };
            },
            Rules : {
                alpha : "^[A-Za-z ]+$",
                alpha_numeric : "^[a-zA-Z0-9_]*$",
                email : "^[^@]+@[^@]+\\.[^@]+$",
                card_number : "[0-9 ]{19}",
                card_cvv : "[0-9]{3}",
                card_date : "(0[1-9]|1[0-2])\\s\\/\\s[0-9]{2}",
                postcode : "^(GIR ?0AA|[A-PR-UWYZ]([0-9]{1,2}|([A-HK-Y][0-9]([0-9ABEHMNPRV-Y])?)|[0-9][A-HJKPS-UW]) ?[0-9][ABD-HJLNP-UW-Z]{2})$"
            },
            validateSingle: function($field){
                // check if theres a pattern specified
                if ($field.attr("pattern")) {
                    //grab the pattern attribute
                    var pattern = $field.attr("pattern");
                    // do we have the pattern defined in our rules?
                    if (typeof Tiga.Validate.Rules[pattern] != "undefined") {
                        rule = new RegExp(Tiga.Validate.Rules[pattern]);
                    // nope? we'll assume it's a regex then
                    } else {
                        rule = new RegExp(pattern);
                    };

                    //test our rule
                    if (rule.test($field.val())) {
                        //pass
                        $field.removeAttr("data-invalid");
                    } else{
                        //fayul!
                        $field.attr("data-invalid", true);
                    };
                // check if field should be equal to another field (e.g. confirm password)
                } else if ($field.attr("data-equalto")) {
                    var compareto = $field.attr("data-equalto");
                    if ($field.val() == $(compareto).val()) {
                        $field.removeAttr("data-invalid");
                    } else {
                        $field.attr("data-invalid", true);
                    };
                };
            },
            validateForm: function(form){
                // innocent until proven guilty
                var valid = true;
                // Let's assess each forms validity one at a time with $.each *cringe*
                $(form.selector + " [required]").each(function(i,o){
                    // checking two things: 1) is the field invalid? 2) is it empty?
                    if ($(o).attr("data-invalid") != undefined || $(o).val().length == 0) {
                        // OBJECTION YOUR HONOUR! This man can't type properly!
                        valid = false;
                    };
                });
                // return validity check
                if (valid) {
                    return true;
                } else {
                    return false;
                };
            },
            cardFormat: function(field, maxlength, fieldtype){
                // set no max value as default
                if (typeof maxlength == "undefined") { maxlength = 9999; /* should be high enough :) */ }
                // cache the caret pos
                var cp = Tiga.Validate.getCaretPos(field);
                //visa or mastercard detection classes for adding icons ( TODO: add more card types )
                if (fieldtype == "card_number") {
                    if (field.value.charAt(0) == 4) {
                        $(field).removeClass("mastercard").addClass("visa");
                    } else if (field.value.charAt(0) == 5) {
                        $(field).removeClass("visa").addClass("mastercard");
                    } else {
                        $(field).removeClass("visa sprite");
                    };
                };
                //remove everything except numbers
                var val = String(field.value).replace(/[^0-9.]/g, "");
                if (typeof fieldtype != "undefined") {
                var spacedval;
                    if (fieldtype == "card_number") {
                            //add spaces
                            spacedval = val.replace(/(.{4})/g,"$1 ");
                            //check to see if we're coming up to a space. if we are, jump the caret over it.
                            if ( [4,9,14].indexOf(cp) != -1 ) {
                                cp++;
                            }
                    }
                    if (fieldtype == "card_date") {
                            //add spaces
                            spacedval = val.replace(/(.{2})/g,"$1 \/ ");
                            //check to see if we're coming up to a space. if we are, jump the caret over it.
                            if ( cp == 2) {
                                cp = cp + 3;
                            }
                            if ( cp == 4) {
                                cp++;
                            }
                    }
                    //enforce max length (use a "maxlength" attribute as well. This just stop the script adding additional charaters)
                    if (spacedval.length > maxlength) {
                        spacedval = spacedval.substr(0, maxlength);
                    };
                    //set the field value to our nicely formatted one
                    field.value = spacedval;
                } else {
                    //enforce max length (use a "maxlength" attribute as well. This just stop the script adding additional charaters)
                    if (val.length > maxlength) {
                        val = val.substr(0, maxlength);
                    };
                    //set the field value to our nicely formatted one
                    field.value = val;
                }
                Tiga.Validate.setCaretPos(field, cp);
            },
            getCaretPos: function(field){
                  // Initialize
                  var iCaretPos = 0;
                  // IE Support
                  if (document.selection) {
                    // Set focus on the element
                    field.focus ();
                    // To get cursor position, get empty selection range
                    var oSel = document.selection.createRange ();
                    // Move selection start to 0 position
                    oSel.moveStart ('character', -field.value.length);
                    // The caret position is selection length
                    iCaretPos = oSel.text.length;
                  }
                  // Firefox support
                  else if (field.selectionStart || field.selectionStart == '0')
                    iCaretPos = field.selectionStart;
                  // Return results
                  return (iCaretPos);
            },
            setCaretPos: function(field, caretPos) {
                if(field != null) {
                    if(field.createTextRange) {
                        var range = field.createTextRange();
                        range.move('character', caretPos);
                        range.select();
                    }
                    else {
                        if(field.selectionStart) {
                            field.focus();
                            field.setSelectionRange(caretPos, caretPos);
                        }
                        else
                            field.focus();
                    }
                }
            }
        };

Example Markup:

<form action="" id="myform">
    <input type="text" required pattern="alpha" placeholder="Name">
    <input type="text" required pattern="alpha_numeric" placeholder="Address">
    <input type="text" maxlength="19" required pattern="card_number" placeholder="Card Number">
    <input type="password" maxlength="3" required pattern="card_cvv" placeholder="CVV">
    <input type="text" maxlength="7" required pattern="card_date" placeholder="Expiry Date">
    <input type="text" maxlength="8" required pattern="postcode" placeholder="Postcode">
    <input type="submit" class="submit">
</form>

<script>
    jQuery(document).ready(function($) {
        $("#myform").tigaValidate();
    });
</script>

Answer 1

Sure, I can give you some pointers. Firstly you mention about the 'annoying' comments, well if you think they're annoying then what use do they have to another person who would be reading the code? I think comments should not really describe line by line what something does, only in exceptional cases where the code looks weird; point out that you're doing something because of a browser bug etc. Adding a comment such as Initialize before a declaration isn't going to help you or a future maintainer.

You can make some improvements to readability when using return. For example:

// return validity check
if (valid) {
    return true;
} else {
    return false;
};

Can simply be this:

return valid;

Similarly:

//validation check on form and enable/disable the button;
if (Tiga.Validate.validateForm(form)) {
    $submit.attr("disabled", false);
} else {
    $submit.attr("disabled", true);
};

Becomes this:

$submit.attr('disabled', !Tiga.Validate.validateForm(form));

As a side note to this point, the final semi-colon in the block isn't necessary. Consider running a tool such as JSHint on your code. As you can see, pasting your code there will give you a ton of suggestions for improvement.

Test against booleans. This line:

if ($(o).attr("data-invalid") != undefined || $(o).val().length == 0) {

Should be:

if ($(o).attr("data-invalid") || !$(o).val().length) {

In that you are testing for a data-invalid attribute (truthy) or that the length is falsey (0).

Performance: Always try to cache the jQuery lookups when you use a selector multiple times in the same block. It's good for minifiers too because something like var $myDiv = $('#myDiv'); can be condensed to var e = $('#myDiv'); and then referred to as e instead of writing the selector string again. An example in your code:

var val = $(this).val().toUpperCase();          
$(this).val(val);

Can be this:

var $this = $(this);
$this.val($this.val().toUpperCase());

And in this case you don't need to cache val in a variable because it's only used in that single place.

Code formatting; try and use braces for your else blocks, it enhances readability:

setCaretPos: function(field, caretPos) {
    if(field != null) {
        if(field.createTextRange) {
            var range = field.createTextRange();
            range.move('character', caretPos);
            range.select();
        }
        else {
            if(field.selectionStart) {
                field.focus();
                field.setSelectionRange(caretPos, caretPos);
            }
            else
                field.focus();
        }
    }
}

Will look much neater like this:

setCaretPos: function(field, caretPos) {
    if (field !== null) {
        if (field.createTextRange) {
            var range = field.createTextRange();
            range.move('character', caretPos);
            range.select();
        } else {
            if (field.selectionStart) {
                field.focus();
                field.setSelectionRange(caretPos, caretPos);
            } else {
                field.focus();
            }
        }
    }
}

It can also be tidied up further:

        // ...
        } else {
            field.focus();
            if (field.selectionStart) {
                field.setSelectionRange(caretPos, caretPos);
            }
        }

Finally, this is a validation plugin so I expected some visual feedback that I was doing something wrong; perhaps a message by each field or even just a red/green border to indicate validity. I'd suggest that some feedback to the user here would make the form more pleasant to fill out.

Hope this helps!

Answer 2

Ben already gave a good answer, but I'll add some thoughts.

First, a note on comments. Since you say you've never worked in a collaborative environment, I'm just happy to see comments of any kind. Usually it's an easy thing to skip when you're just writing code "for yourself". Ben is still right that the commenting style could be improved here and there (especially if you yourself find it "annoying"). But overall, it's fine. For instance, this one actually explaining something I was about to comment on:

// ..traverse the DOM and look for required fields, bind validation to the blur event
// (all traversal accomplished with sizzle when possible - http://jsperf.com/jquery-find-vs-sizzle/3)
$(form.selector + " [required]").on("blur", function(e){ 

At first, just reading the code, I thought "why not use form.find()? But that comment cleared it up nicely.

... but if you look at that jsperf, you'll see that there isn't actually a huge benefit to going this route. Sure, it's a little bit faster, but I doubt it's enough to justify code that made someone like me go "huh?" when reading it.

However, the jsperf does show a significant boost when using a scoped $.find(), so you could do:

$.find("[required]", $form[0])

But that's a micro-optimization.

Another thing I noticed, is that you use the HTML5 pattern attribute for your own ends. This is A Bad Idea™. It only works because you're disabling the browser's own validation with the novalidate option. But if, for whatever reason, built-in validation is turned on again, or your code breaks or isn't loaded, or the user has JS turned off (which doesn't disable built-in validation), the browser will still honor those patterns. And suddenly the user is unable to input, say, a credit card number that doesn't literally include the string "card_number", because that's the only thing that the pattern "card_number" will ever match.

Much cleaner to use a data-* attribute, like, say data-pattern when the pattern isn't an actual regex pattern.

Back to JS. You've got a fair number of very short variable names like o or cp. No need for that. Be more descriptive - it'll also save you some comments.

Another thing I noticed is this thing:

if ($(this).is("[pattern='card_date']")){
    Tiga.Validate.cardFormat(this, 7, "card_date");
}
if ($(this).is("[pattern='card_number']")){
    Tiga.Validate.cardFormat(this, 19, "card_number");
}
if ($(this).is("[pattern='card_cvv']")){
    // simplified version just automatically strips out letters on keyup
    Tiga.Validate.cardFormat(this);
}

There are few things to unpack here:

1. If the 3 code branches are mutually exclusive, it'd be more explicit if you used a switch or else if. Or use a lookup (like your Rules object), to take the appropriate action.

2. cardFormat() is doing way too much, if you ask me. And I haven't even looked at the function. But just the fact that it apparently handles dates, card numbers and CVVs tells me it's doing too much. Sure, those things all have to do with credit cards, but their formatting is entirely independent.

3. The code displays a strange mix of configuration, duplication and hard-coded values. The HTML will contain the pattern to use (configuration), yet you duplicate the string when passing it on to cardFormat, and you pass along a (hard-coded) numeric value that, out of context, is completely opaque. Oh, "card_number" implies 19? Uh, ok? What is 19 again? If it's so necessary and always follows from the pattern, why isn't it hard-coded in cardFormat instead, since you pass it the pattern string anyway? Alternative, if it's meant to be configurable, why is hard-coded here?

There are many ways to address these things. Personally, I'd probably create an object for credit card formatting functions, named for their speciality (like your do for the regexes in Rules).

I've got to go, but those are my initial thoughts. Overall it's not bad, but there's room for improvement.