Simple form validation in javascript

Question

Used to use Laravel's easy to use form validation class, decided to mimic it in JS. I've been learning JS for 4 days now and decided to write my first piece of real code.

I'm pretty sure its full of bad practices, and a lot of it can be improved. I'm looking for some constructive criticism on what I can do better next time, thanks in advance.

Class:

class Validator {
 make(rules) {
 this.rules = rules;
 }
 valid() {
 if (!this.hasOwnProperty('rules')) {
 return true;
 }
 let errors = [];
 let checkFunction = this.checkRuleForElement; // Storing this here because it doesn't seem to work in its invoked scope.
 Object.keys(rules).forEach(function(key) {
 let formElement = document.getElementsByName(key)[0];
 if (formElement == undefined) {
 return;
 }
 let ruleSet = rules[key].split('|');
 ruleSet.forEach((rule) => {
 let result = checkFunction(rule, formElement)
 if (!result.valid && result.message != undefined) {
 errors.push(result.message);
 }
 });
 });
 return {
 valid: errors.length == 0,
 errors: errors
 }
 }
 checkRuleForElement(rule, formElement) {
 let isValid = true;
 let error = "no error";
 switch (rule) {
 case "required":
 if (formElement.value.length <= 0) {
 isValid = false;
 error = `Form element ${formElement.getAttribute("name")} is required.`;
 }
 break; 
 // todo: alpha => only alpha characters
 // todo: alphanum => Alphanumeric characters
 // todo: num => Numbers only
 }
 return {
 valid: isValid,
 message: error
 };
 }
}

Setup (usage):

let validator = new Validator();
function checkValidation() {
 validator.make({
 "username": "required",
 "password": "required",
 });
 let result = validator.valid();
 if (!result.valid) {
 alert('Something went wrong.');
 alert('Errors are: ' + result.errors[0]);
 }
 else {
 alert('Validation was a success.');
 }
}

• Zeta: Welcome to Code Review. For better reviews, I suggest you to add some more information what your code does. Not everyone knows Laravel, so an independent description can improve your chances on a good review.

Answer 1

I must admit this is pretty good code for someone that has just started, I like that you use the more "functional" style Javascript allow you to use . So good job :) But there are some design issues I see with the code.

1. Factory vs constructor

Your Validator has a make method which takes the rules in, It is basically used as a factory method (A factory is a design pattern if you are not familuar with it). But it has none of the benefits a factory gives you, since it is a instance method.

Having to instantiate a Validator object and then being forced to call make is cumbersome. Instead I suggest you make the make function static and add a constructor

class Validator
{
 static make(rules) 
 {
 return new Validator(rules);
 }
 constructor(rules) 
 {
 this.rules = rules;
 }
}

2. Checking class properties

You don't need to do this check in your valid function.

if (!this.hasOwnProperty('rules')) {
 return true;
}

Since it's practically not possible to not have rules with the proposed factory. I understand why this is here though. You wanted to make sure you can't forget the make call after instantiating a Validator object. But that's a wrong schematic. If the caller uses your code wrong it should fail, and not say "I don't care, but this is valid"

3. Naming

Methods should indicate that they are actions, and properties that they are values.

Your valid method sounds a lot like a property, like a value. I would call it validate since that is a verb and makes it clear that it does something and not "is" something. And I would rename the property valid that your return in your valid method to isValid

4. ES6

EDIT: I noticed to late that you already used an arrow function in your code. Ignore the explanation if you want and just replace the Object.keys().forEach(callback) callback with an arrow function. BTW you do not need the parentheses if you only have one parameter.

You can use a arrow function in your forEach(function) function. This is a es6 feature and a bit more advanced tho. Using functions is fine but it can solve the issue you have with scoping.

The function keyword does create a new scope and therefore changes the meaning of this. The arrow function on the other hand behaves exactly like a function but does not change the meaning of this

validate() {
 let errors = [];
 Object.keys(rules).forEach(key => {
 let formElement = document.getElementsByName(key)[0];
 if (formElement == undefined) {
 return;
 }
 let ruleSet = rules[key].split('|');
 ruleSet.forEach((rule) => {
 let result = this.checkRuleForElement(rule, formElement)
 if (!result.valid && result.message != undefined) {
 errors.push(result.message);
 }
 });
 });
 return {
 isValid: errors.length == 0,
 errors
 }
}

Another thing you can leave out is the errors: errors part. You can shorten it to just errors since the name of the variable is the same as key of the object

5. Decouple things

Currently your "rule running" and "rule definitions" are all in the same class/method (in the switch in checkRuleForElement) So if you want to add a new rule you have to modify your Validator class which should only do one thing: Invoke the rules for a element. The rule definitions are a whole separate concern.

You can try to decouple it by using arrays.

A sample implementation may look like this:

// This is not good code, take it as an "idea" or psuedocode

class Validator
{
 static registeredRules = [];
 static registerRule(name, run)
 {
 Validator.registeredRules.push({ name, run });
 }
 checkRuleForElement(ruleName, formElement)
 {
 var rule = Validator.registeredRule.find(element => element.name === ruleName);
 return rule.run(formElement);
 }
}

6. Abstract things

A thing you may want to consider is that there may be multiple forms or multiple elements, with your current implementation you are relying on the fact that there are is a unique element foreach "rule". Also you are interacting with the Dom directly which makes things hard to change when that assumption changes.

You could solve this by, for example using Dom elements as an input:

Validator.make({
 document.querySelector("#user_input"): "required"
})

Bonus exercises

You can try to add a more advanced "rule parser" and add parameters for rules.

This could look like this:

Validator.make({
 "username": "required|min:5"
})

Another thing you could try is, skipping this whole "naming" thing and using just functions

For example

const required = element => element != undefined;
// If you do not understand this Syntax, it's basically a function that returns a function.
const min = count => element => element.length > count
Validator.make({
 "username": [required, min(5)]
});

This would give you several advantages. For example that your rules can be checked by a static analyzer, If you use strings there is always a chance you mistype something, like use "regired" instead of "required" and not even notice it, with functions on the other hand you will get an error if you use something that is undefined. Also you could reuse rules better.

---

Have fun :)

I wrote this from my mobile so if there are any mistakes let my know. I will most likely add more when I get my hands on a pc!

Answer 2

Keep it simple

There is an awful lot of arguments being passed and objects being created and returned, on top of 3 exposed functions that are just middle men complicating the core need.

You want to check a rule set and see if it valid, if not then see the errors.

This can all be done via getters for isValid and errors and a setter for the rule set that also initiates the validation.

You can use Object.entries to get the key and value pairs

 function Validator(ruleSet) {
 const errors = [];
 const rules = {
 required(element) {
 if (element.value.length <= 0) {
 errors.push(`${element.getAttribute("name")} is required.`);
 }
 },
 // todo: alpha => only alpha characters
 // todo: alphanum => Alphanumeric characters
 // todo: num => Numbers only
 }
 const API = {
 set rules(ruleSet) { 
 errors.length = 0;
 Object.entries(ruleSet).forEach(([key, value]) => {
 const element = document.getElementsByName(key)[0];
 if (element) {
 value.forEach(rule => rules[rule] && rules[rule](element));
 }
 });
 },
 get isValid() { return errors.length === 0 },
 get errors() { return [...errors] },
 };
 if(ruleSet) { API.rules = ruleSet };
 return API;
}

Instantiating the Validator with a rule set and getting the results without the need to force the validation

// Usage 
const validator = Validator({
 username: ["required"],
 password: ["required"],
});
if(!validator.isValid){
 alert('Errors are: \n' + validator.errors.join("\n"));
}

But why create a complex object

You create an object that does not actually contain the information you are after. All you want is the result {isValid, errors}

It would be more practical as a basic function

function validator(ruleSet) {
 const errors = [];
 const rules = {
 required(element) {
 if (element.value.length <= 0) {
 errors.push(`${element.getAttribute("name")} is required.`);
 }
 },
 }
 Object.entries(ruleSet).forEach(([key, value]) => {
 const element = document.getElementsByName(key)[0];
 if (element) {
 value.forEach(rule => rules[rule] && rules[rule](element));
 }
 });
 return {isValid : errors.length === 0, errors};
}

BTW the split("|") is a little hacky and limits you rules to be strings. Using arrays to hold the values give more flexibility.

const result = validator({
 username: ["required"],
 password: ["required"],
});
if(!result.isValid){
 alert('Errors are: \n' + result.errors.join("\n"));
}