Simplifying this form validation script

Question

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>

• Piotr Kula: 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

Answer 1

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

Answer 2

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!

• Leo: Shall I delete validator = this; var currentmsg =this; then mate?
• konijn: I think you should, and simply point to this in all cases.
• Leo: 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