patternjavascriptMinor
Simplifying this form validation script
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
```
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
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
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) {
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
varContext
StackExchange Code Review Q#40964, answer score: 4
Revisions (0)
No revisions yet.