JS validation and submission with AJAX

I've got an assignment to do pure JS validation as well as submit with AJAX. Here is the code I've got so far. I'm wondering if I can do away with the whole "reason" bit. That was because it was originally meant to display a list of all the errors, but I've tweaked the code to instead display the errors right by the field.

Basically I want to call up the validateFormOnSubmit and just have it go through all the if statements instead of all the separate functions, then if it all passes execute submitFormAjax.

The AJAX is currently not working but don't want to troubleshoot that until I've cleaned up the JS code.

JSfiddle

HTML



First Name




Nickname



Email




Phone




I prefer
Dogs
Cats
Neither



My favorite number between 1 and 50




Disclaimer
I confirm that all the above information is true.



Send




JS

`function validateFormOnSubmit(contact) {
reason = "";
reason += validateName(contact.name);
reason += validateEmail(contact.email);
reason += validatePhone(contact.phone);
reason += validatePet(contact.pet);
reason += validateNumber(contact.number);
reason += validateDisclaimer(contact.disclaimer);

console.log(reason);
if (reason.length > 0) {

return false;
} else {
// Show some loading image and submit form
submitFormAjax();
}
}

// validate required fields
function validateName(name) {
var error = "";

if (name.value.length == 0) {
name.style.background = 'Red';
document.getElementById('name-error').innerHTML = "The required field has not been filled in";
var error = "1";
} else {
name.style.background = 'White';
document.getElementById('name-error').innerHTML = '';
}
r

From a once over:

-
You are repeating .style.background = 'Red'; and .style.background = 'White'; a ton, what if suddenly all these fields also have to get bold. I would suggest a function like highlightField that does the setting of the style:

function setHighlight( element , isHighlighted )
{
  element.style.background = isHighlighted ? 'Red' : 'White';
}

• The reason being a number is silly, since you do not do anything with the number, I would simply return booleans and AND these booleans.

• It is considered a better style to have one single var statements, with comma separated variables.

-
Then, you could change this:

function validateEmail(email) {
    var error = "";
    var temail = trim(email.value); // value of field with whitespace trimmed off
    var emailFilter = /^[^@]+@[^@.]+\.[^@]*\w\w$/;
    var illegalChars = /[\(\)\\,\;\:\\\"\[\]]/;

    if (email.value == "") {
        email.style.background = 'Red';
        document.getElementById('email-error').innerHTML = "Please enter an email address.";
        var error = "2";
    } else if (!emailFilter.test(temail)) { //test email for illegal characters
        email.style.background = 'Red';
        document.getElementById('email-error').innerHTML = "Please enter a valid email address.";
        var error = "3";
    } else if (email.value.match(illegalChars)) {
        email.style.background = 'Red';
        var error = "4";
        document.getElementById('email-error').innerHTML = "Email contains invalid characters.";
    } else {
        email.style.background = 'White';
        document.getElementById('email-error').innerHTML = '';
    }
    return error;
}

to

function validateEmail(email) {
    var emailFilter  = /^[^@]+@[^@.]+\.[^@]*\w\w$/,
        illegalChars = /[\(\)\\,\;\:\\\"\[\]]/,
        errorMessage = '',
        isValid;

    //Do we have an error ?
    if (email.value == "") {
        errorMessage = "Please enter an email address.";
    } else if (!emailFilter.test(trim(email.value))) { //test for illegal chars
        errorMessage = "Please enter a valid email address.";
    } else if (email.value.match(illegalChars)) {
        errorMessage = "Email contains invalid characters.";
    }

    document.getElementById('email-error').innerHTML = errorMessage;
    isValid = !errorMessage.length;
    setHighlight( email , !isValid );
    return isValid;
}

You could do this for every validation rule.

• You must declare all your variables with var so that you do not pollute the global namespace ( I am looking at you, reason = "" )

-
If all validation functions will return a boolean, than you could rename reason to isValid:

var isValid = validateName(contact.name)     &&
              validateEmail(contact.email)   &&
              validatePhone(contact.phone)   &&
              validatePet(contact.pet)       &&
              validateNumber(contact.number) &&
              validateDisclaimer(contact.disclaimer);

obviously the && does not have to be aligned, but it makes my heart beat faster :)

• There would still be the concern that the name validateXxx lies a little because it also returns a boolean, but since it also updates the UI and does the validation it would be hard to name this correctly without having a really long name.

StackExchange Code Review Q#42182, answer score: 4