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.

Can anyone please help me to simplify this form validation script? It works great but I was just wondering if I can get some help to make it simpler. Your opinion on the approach I used below is also welcome (i.e it is good or bad practice/method).

<script type="text/javascript">

        var FormValidation = function(form){


             this.messages = {

                required    : 'This field should not be empty',
                email       : 'Please enter  valid email',
                number      : 'Please enter  valid number',
                min         : 'This field length should be minimum ',
                max         : 'This field length should not exceed ',
                range       : 'This field length between  '
            };

            validator = this;

            var currentmsg =this;

            this.required = function(value){
                var valid = (value.length > 0);
                return {status: valid, message: valid ? '' : currentmsg.messages.required};
            }

            this.email = function(value){
                var pattern = /^[a-zA-Z0-9._-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4}$/ ;
                var valid = pattern.test(value);
                return { status:valid, message: valid ? '' : currentmsg.messages.email};
            }

            this.number = function(value){
                var pattern = /^[0-9]+/ ;
                var valid = pattern.test(value);
                return { status:valid, message: valid ? '' : currentmsg.messages.number};
            }

            this.min = function(value,args){
                if(value.length > 0){
                    var valid = (value.length >= args[0])
                    return { status:valid, message: valid ? '' : currentmsg.messages.min + args[0] };
                }
            }

            this.max = function(value,args){
                    if(value.length > 0){
                        var valid = (value.length <= args[0])
                        return { status:valid, message: valid ? '' : currentmsg.messages.max + args[0] };
                    }
                }

            this.range = function(value,args){
                var valid = (value.length >= args[0] && value.length <= args[1])
                return { 
                    status:valid, 
                    message: valid ? '' : currentmsg.messages.range + args[0] + ' and ' + args[1]
                    };
            }

            this.validators = {
                required : validator.required,
                email    : validator.email,
                number   : validator.number,
                range    : validator.range,
                max      : validator.max,
                min      : validator.min 
            };

            this.validate = function(form){
                var elements = form.elements;
                var status = true;
                for(var i = 0; i < elements.length ; i++){
                    var validate = elements[i].getAttribute('validate');
                    if(!validate){      
                        continue;
                    }
                    var types = validate.split(' ');
                    for(var j = 0; j < types.length; j++){
                        var result = this.validateField(elements[i], types[j]);
                        if(!result) { 
                            continue 
                        }
                        this.displayErrors(elements[i], result);
                        if(!result.status) {                            
                            status = false;
                            break;
                        }
                    }
                }
                return status;
            }

            this.displayErrors = function(element, result){
                element.className = result.status ? '' : 'error';
                var elErr =element.errorMsg;
                if(elErr!=null)
                    elErr.className = result.status ? '' :'valerr'
                    if(!element.errorMsg){
                        elErr = document.createElement("div");
                        elErr.id = 'valerr';
                        element.parentNode.appendChild(elErr);
                        element.errorMsg = elErr;
                    }
                elErr.innerHTML = result.message;
            }

            this.validateField = function (element, type){
                var validationType = type;
                var args ;
                if(type.indexOf("(")!= -1 && type.indexOf(")") != -1){
                    var result = this.getArguments(type);
                    validationType = result.type;
                    args = result.argsList;
                }
                validator = this.validators[validationType];
                if(validator != null){
                    return validator(element.value ,args);
                }
                return null;
            }

            this.getArguments = function(type){
                var validationtype = type.substring(0,type.indexOf("("));
                var args = type.substring(type.indexOf("(")+1,type.indexOf(")")).split(',');
                return { type : validationtype, argsList : args}
            }

            this.init = function() {
                var curForm = this;
                var forms = document.forms;
                for(var i = 0; i < forms.length ; i++){
                    if(forms[i].getAttribute('validate')!=null){
                        forms[i].onsubmit = function(){
                            return curForm.validate(this);
                        };
                    }
                }       
            }
    }

    window.onload = function() {        
        var formValidation = new FormValidation();
        formValidation.init();
    }
    </script>
    <form method="post" action="#" validate>

        <table>
            <tr>
                <td>Name :</td>
                <td><input type="text" name="" validate="required min(5)"></td>
            </tr>
            <tr>
                <td>Mobile No :</td>
                <td><input type="text" name="" validate="required number min(10) max(10)">
            </tr>
            <tr>
                <td>Email :</td>
                <td><input type="text" name="" validate="required email"></td>
            </tr>
        </table>
        <input type="submit"  value="submit"/>
    </form>
share|improve this question
1  
If that is meant to be jQuery plugin you are not wrapping it correctly into the jQuery namepsace. Could cause issues but its jsut precautin –  ppumkin Feb 5 at 16:37
add comment

2 Answers 2

up vote 4 down vote accepted

Spent a little hacking away at this to simplify the code. I wrote comments on changes inline the script, let me know if you have any questions

Some thoughts on the changes. Notice that how I restructured your validators in the form

validatorName: {
   message: 'fail msg',
   classes: 'add classes to element on fail',
   test: function(){return true;}
};

I've made some validators of my own and this is my personal preference for structuring validators and it allows us to due away with that weird currentMessage variable.

I've also fixed a bunch of your linting errors - there were a lot of them so I didn't really comment on them. In the future run your code through jshint before posting it :)

One last thought. There doesn't seem much of a reason for you to be writing this code as a class as its most likely a singleton and you're not writing the code on the prototype. I've resturctured your code to be a more conventional singleton rather than a class

Here's the start of a counter proposal... Theres likely bugs but the code is a great deal simpler and uses less hackery than your original approach.

var FormValidation = function (form) {

    //Remove Gloabl variable and duplicate as mentioned by @konijn
    //replaces validator and currentMessage
    var self = {};

    //Counter proposal: I suggest you have an object of validators instead of
    // Dividing the parts of validators between messages/validators/this. Centeralize
    // all parts in a single object
    //Structure of a validator {test: fn(value, args) -> bool, message:str, classes:str}
    // Advantages: more explicit and you dont need that ugly current message variable
    self.validators = {
        required: {
            message: 'This field should not be empty',
            classes: 'reqerr',
            test   : function (value) {
                return value.length > 0;
            }
        },
        email: {
            message: 'Please enter  valid email',
            test   : function (value) {
                //no need for a pattern var, this is explicit as it gets
                return /^[a-zA-Z0-9._-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4}$/.test(value); //comment what is a valid email or a link where you got the regexp
            }
        },
        number: {
            message: 'Please enter  valid number',
            classes: 'valerr',
            test   : function (value) {
                return /^\d+$/.test(value); //simpler number regexp and ensures entire string is numbers not just start characters
            }
        },
        min: {
            message: 'This field length should be minimum ',
            test: function (value, args) {
                return value.length > 0 && value.length >= args[0];
            }
        },
        max: {
            message: 'This field length should not exceed ',
            test   : function (value, args) {
                return value.length > 0 && value.length <= args[0];
            }
        },
        range: {
            message: 'This field length between ',
            test   : function (value, args) {
                return value.length >= args[0] && value.length <= args[1];
            }
        }
    };

    var getArguments = function (type) {
        var validationtype = type.substring(0, type.indexOf("("));
        var args = type.substring(type.indexOf("(") + 1, type.indexOf(")")).split(',');
        return {
            type    : validationtype,
            argsList: args
        };
    };

    var displayErrors = function (element, validator) {
        element.className = validator.classes || 'error';
        var elErr = element.errorMsg;
        if (!element.errorMsg) {
            elErr = document.createElement("div"); //dont set the id if you're not gonna use it
            element.parentNode.appendChild(elErr);
            element.errorMsg = elErr; //Warning this can make old ie very slow to unload
        }
        elErr.innerHTML = validator.message;
    };

    //This is your old validateField function modified to show the error if it fails in order to simplify the validate function
    var validateField = function (element, type) {
        //no need to make a var equal to a param
        var valid = true;
        var args, validator;
        //better to use a regexp here to get args
        if (/\(.*\)/.test(type)) {
            var result  = getArguments(type);
            type        = result.type;
            args        = result.argsList;
        }
        validator = self.validators[type];
        if (validator) { //no need for the != null
            valid = validator.test(element.value, args);
            if (!valid) {
                //display errors
                displayErrors(element, validator);
            }
        }
        return valid;
    };

    self.validate = function (form) {
        var elements = form.elements;
        var status = true;
        var element, validate, types, result, //create variables outside the loop
            i, j;
        for (i = 0; i < elements.length; i++) {
            element = elements[i];
            validate = element.getAttribute('validate');
            if (!validate) {
                continue;
            }
            types = validate.split(' ');
            for (j = 0; j < types.length; j++) {
                if (types[i] && //not empty string and
                !validateField(element, types[j]) //if not valid
                ) {
                    status = false;
                    break;
                }
            }
        }
        return status;
    };

    self.init     = function () {
        var forms = document.forms;
        for (var i = 0; i < forms.length; i++) {
            if (forms[i].getAttribute('validate') != null) {
                forms[i].onsubmit = function () {
                    return self.validate(this); //this refers to the current form
                };
            }
        }
    };

    return self;
};

Here's a demo of your code up and running: http://jsbin.com/ropi/1/edit

share|improve this answer
    
Thank you very much man!!! –  Leo Feb 5 at 22:52
add comment

This is already quite tight, I like it.

There is one big thing that jumps out:

validator = this;
var currentmsg =this;

You are using this, and validator and currentmsg all really pointing to this, that is making it harder than needed. Furthermore you escape scope by not using var for validator!

share|improve this answer
    
Shall I delete validator = this; var currentmsg =this; then mate? –  Leo Feb 5 at 17:10
    
I think you should, and simply point to this in all cases. –  konijn Feb 5 at 17:12
    
Please can you help me do it? I tried but can't figure out how to exactly do it. I am not really expert in JavaScript and had to get lot of help to get the above working –  Leo Feb 5 at 17:15
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.