HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Simplifying this form validation script

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisscriptvalidationsimplifyingform

Problem

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).

```

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] && value.length




Solution

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] && 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) {

Code Snippets

validatorName: {
   message: 'fail msg',
   classes: 'add classes to element on fail',
   test: function(){return true;}
};
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

Context

StackExchange Code Review Q#40964, answer score: 4

Revisions (0)

No revisions yet.