How can I upgrade my validation in JavaScript?

Question

How to improve my validation in JavaScript?

Namely I am wondering whether some parts of code can be shortened, improved, or removed althogether.

I don't want to change the ES5 syntax because so far I am not familiar with Es6 syntax.

I also want to avoid using jQuery.

This is my validation code: https://codepen.io/RobotHabanera/pen/NWbKJLe

var form = document.querySelector('form'); var errormessage = document.querySelector('.error-message'); form.addEventListener('submit', function(e) { e.preventDefault(); // musimy zalożyć czy formularz jest poprawnie wypełniony czy nie var errors = []; errormessage.innerHTML = ''; var clearAllParagraphsAtTheStart = document.querySelectorAll('.form-group p'); clearAllParagraphsAtTheStart.forEach(function(item) { item.innerHTML = ''; }); // sprawdzić czy walidacja bedzie działać for (var i = 0; i < form.elements.length; i++) { var field = form.elements[i]; switch (field.name) { case 'email': // najpierw negujemy funkcje hasMonkeyInFiled i jesli negacja przebiegnie prawidłowo to wykona się prawa strona komunikatu && bo // jesli negacja nie przebiegnie prawidłowo to linijka z prawej sterony sie nie wykona. !hasMonkeyInFiled(field) && errors.push({ email: 'Email musi posiadać znak @' }); break; case 'name': !hasMoreThanTwoChars(field) && errors.push({ name: 'Twoje imię jest za krótkie' }); break; case 'surname': !hasMoreThanSixChars(field) && errors.push({ surname: 'Twoje nazwisko jest za krótkie' }); break; case 'pass1': !hasCorrectPassword(field, form.elements[i + 1]) && errors.push({ pass1: 'Hasła nie są takie same lub puste' }); break; case 'pass2': !hasCorrectPassword(field, form.elements[i - 1]) && errors.push({ pass2: 'Hasła nie są takie same lub puste' }); break; case 'agree': !isChecked(field) && errors.push({ agree: 'Musisz zaakceptować warunki' }); break; } } if (errors.length) { e.preventDefault(); errors = errors.filter(function(v, i, a) { return a.indexOf(v) === i; }); errors.forEach(function(item, index) { // wklej wartość klucza z tablicy errors gdzie nazwa klucza jest równa wartości danego data-validation bierzącego inputa z pętli var currentKey = Object.keys(errors[index]); var currentValue = Object.values(errors[index]); var inputs = document.querySelectorAll('.form-group input'); inputs.forEach(function(item, index) { if (item.dataset.validation == currentKey) { var elementP = document.createElement('p'); elementP.innerHTML = currentValue; item.after(elementP); } }); /* errormessage.append(elementP); */ }); } }); function hasCorrectPassword(field1, field2) { if (hasMatch(field1, field2) && hasNumberChar(field1) && hasNumberChar(field1)) { return true; } return false; } // paramertr 'field' to jest input element function hasMonkeyInFiled(field) { return field.value.indexOf('@') > -1; } // input ma wiecej niz 6 znakow function hasMoreThanSixChars(field) { return field.value.length > 6; } // input ma wiecej niz 2 znaki function hasMoreThanTwoChars(field) { return field.value.length > 2; } // checkbox musi byc zaznaczony function isChecked(field) { return field.checked; } // pierwsze i drugie hasło są identyczne ale nie puste function hasMatch(field1, field2) { // jak sprawdzic czy nie sa puste if (field1.value.length && field1.value.length) { return field1.value == field2.value; } return false; } // Warunek dla chętnych. Dodatkowe. Hasło ma mieć co najmniej 6 znaków (w tym co najmniej jedną liczbę i jedną literę) function hasNumberChar(field) { var numbers = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]; var hasNumber = false; numbers.forEach(function(number) { if (field.value.indexOf(number) > -1) { hasNumber = true; } }); return hasNumber; } function hasLetterChar(field) { var chars = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']; var hasChar = false; chars.forEach(function(char) { if (field.value.toLowerCase().indexOf(chars) > -1) { hasChar = true; } }); return hasChar; } // input ma wiecej niz 5 znakow function hasMoreThanFiveChars(field) { return field.value.length > 5; } function isPangram(string) { string.replace(/ /g, ''); for (var i = 0; i < string.trim().length; i++) { var array = []; var chars = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']; chars.forEach(function(char) { if (char == string.substr(string[i], 1)) {} else { array.push(string[i]) } }); } if (array.length) {} // bierze pierwszą litere z tablicy i jedzie tą literą po wszystkich znakach tekstu string.replace(/ /g, ''); chars.forEach(function(char) { for (var i = 0; i < string.length; i++) { var chars = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']; if (char == string[i]) { console.log('jest'); } else { console.log('nie ma'); } } }); }; function isPangram(string) { string.replace(/ /g, ''); var array = []; var chars = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']; chars.forEach(function(char) { for (var i = 0; i < string.length; i++) { // każda litera alfabetu ma przejechać po tekście if (char == string[i]) { array.push(string[i]); } else {} } if (array.length > 0) { console.log('jest') } }); }; 

Answer 1

The validation functionality

Overall the validation function looks reasonable. You have a preventDefault at the beginning that blocks the form from ever being sent, but other than that it looks logically structured. You first clear all your previous errors, then look if there are any new errors, and finally show the new errors. You can consider factoring out those three parts, but I don't think it is needed in this particular case.

There are however some issues with the functions you call though, and some false assumptions you make in and around them.

hasMoreThanTwoChars

Whenever you find yourself writing numbers inside function names, it usually means you are doing something wrong. There is either a better description (e.g. fibonacci instead of addLastTwoItemsOfSequence) or in your case, you can and should factor out the number to a parameter.

In your case you can write a function hasMinimumNumberOfCharacters(field, numberOfCharacters) that handles all three variants.

name and surname

While the following article is more than ten years old, it is still very much applicable: Falsehoods Programmers Believe About Names. For example, I can't fill in your form without padding my name with spaces or something weird like that.

hasCorrectPassword

You call your hasCorrectPassword function with form.elements[i - 1] and form.elements[i + 1]. You are almost guaranteed that this will break at some point due to re-ordering or adding of fields. Make it more robust by referencing the fields themselves by name, for example through document.querySelector('#pass2').

Furthermore, the function itself is odd, because it performs two functions. You should probably split it up in two well-defined functions. One is isEqualToField(field1, field2) and one is containsNumber(field). If your password requirements become more complex you could instead consider isValidPassword(field) and isEqualToField(field1, field2).

You also seem to be checking twice if the same field has a number character.

And finally if you have an if-else statement that either returns true or false, you can just return the condition of the if-else statement itself.

function hasCorrectPassword(field1, field2) { return hasMatch(field1, field2) && hasNumberChar(field1); } 

hasMatch

Always check equality using ===. Always be mindful of the types of variables and never rely on automatic type conversion for your code to work.

hasNumberChar

This function can be improved in many ways. numbers is an array of Number, and you rely on String.indexOf to cast the items to a string. You could make it an array of String to be more mindful when you are using String and when you are using Number. Additionally, it is not very nice to have to define a whitelist of characters, but I suppose we will not use any other digits than the 10 we have defined here.

Instead of Array.forEach which requires you to loop through all items, you could use several other methods that allow you to return early.

Array.some(func) and Array.every(func) return a boolean if respectively one or all of the items in the array meet a condition, and return early. You would rewrite it as

digits.some(function (digit) { // This string contains the digit we are looking for return field.value.indexOf(digit) !== -1; }); 

You can rewrite forEach loops with a for(const x of myArray) as well, with the added benefit that you can return from this loop or break out of the loop.

for (const digit of digits) { if (field.value.indexOf(digit) > -1) { return true; } } return false; 

Both of these approaches have as additional benefit that you are not keeping track of hasNumbers, but return instantly when you know the answer.

hasLetterChar

This function has the same problems as the previous one, except that we have one more problem. Your language contains many characters that do not match your whitelist and relying on that whitelist will cause problems at least for a subset of your users. Even in languages where characters outside this whitelist are uncommon or non-existent should not rely on such a whitelist. For the why, I refer again back to the article a few sections above here.

If you decide to keep it at all, consider if a check if it contains anything but whitespace would suffice (value.match(/\S/)). Otherwise look into something like this question on Stackoverflow, but realise that you will likely miss entire scripts and people will be annoyed they can't enter their name because of overly restrictive validators.

isPangram

The function isPangram is defined twice, and twice it does not return a boolean. Since it serves no purpose, it should be removed.

Both functions contain weird things like empty if or else statements, issues like not using strict equality === as mentioned before and the odd whitelist.

Use let instead of var

Even if you use nothing else than this from modern javascript, use let when declaring variables instead of var. Variables declared with var can be read and written to outside the context you would expect them to be used. An example of that is variables defined with var inside loops being accessible outside that loop. Using let will magically fix that by only being available to read and write once you define it, and properly being discarded at the end of the block it is defined in. Some heavy reading about scoping can be found in What is the scope of variables in Javascript.

function testWithLet() { // console.log(x); // Uncaught ReferenceError: x is not defined // x += 5; // Uncaught ReferenceError: x is not defined for (let i = 0; i < 5; i++) { const x = 1; } // console.log(i); // Uncaught ReferenceError: i is not defined } function testWithVar() { console.log(x); // undefined x += 5; // What did this just do? for (var i = 0; i < 5; i++) { var x = 1; } console.log(i); // 5 } testWithLet(); testWithVar();

Some additional notes based on the codepen

Keep in mind that the browser already has built-in functionality both for validating and making it easier to fill in form fields. In particular, use type to define things like email, tel or number. You can (but don't have to) define patterns the input should be adhering to. (docs on mdn)

Use the inputmode attribute to give a hint to the browser which on-screen keyboard they should use. This is in particular nice for the fast majority of people who use a mobile device to use the form. Valid values include text, tel, email and url. (docs on mdn)

Labels should be associated with their form fields. Either use <label>label text: <input /></label> or the for=".." attribute on the label to link it to an input. You do this so people with screenreaders have any idea what they are looking at, and to allow people to select the field by clicking the label.