Form validation appears to be too rigid

Question

A Stack Overflow coder said my code is too rigid and inflexible. Would someone critique my form validation code, and give some pointers on being more efficient?

Code Logic:

If the field has an error (empty or a RegEx fail) with either on submit or on change, the user will see a suitable message indicating the error and a Unicode arrow pointing to the invalid field.
If the the field has passed, the arrow and error message are not displayed.

Link to JsFiddle Code Sample

    <!doctype html>
    <html lang="en">
    <head>
    <meta charset="UTF-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <title>form validation</title>
    <style type="text/css">
    body, h1, input, div {
       font-family: arial;
       }
    input[type="text"],
    input[type="password"] {
       font-size: 1.15em;
       padding: 0.4em 0.5em;
        }
    /* Start red arrow error messages below input fields */
    .error-text-login{
        color: #ea4040;
        vertical-align: bottom;
        text-align: middle;
        width: 315px;
        display: none;
    }
.error-text-login:before {
    content: '\0020 \21E1 \0020';
    vertical-align: bottom; 
    }
    .error-text-login {
       font-size: 0.75em;   
    }
    .error-text-login:before {
        font-size: 1.3em;   
    }
    /* End red arrow error messages below input fields */
    #usernameLogin.error input,
    #passwordLogin.error input {
        background-color: #fff7f7;
        border: 1px solid #f68c8c;
        border-radius: 2px;
        box-shadow: 0 0 6px #ffeded inset;
        color: #e10000;
    }

    #usernameLogin.default input,
    #passwordLogin.default input {
        background-color: #f7f7f5;
        border: 1px solid #c1c1c1;
        border-radius: 2px;
        box-shadow: 0 0 6px #eeeeee inset;
        color: #a3a3a3;
    }
    </style>
    </head>
    <body id="top">
        <div class="box" style="float: left; width: 47%; margin-right: 6%; height: 200px;">
            <h1>Login</h1>
                <form id="theForm" method="post" action="index.html" >
                <p id="usernameLogin" class="default">              
                    <input type="text" placeholder="Username/Email" id="userName" name="userName" >
                        <div id="name-error" class="error-text-login"></div>    
                </p>                        
                <p id="passwordLogin" class="default">
                    <input type="password" name="pswd" id="pswd" placeholder="Password"  >              
                        <div id="pswd-error" class="error-text-login"></div>

                    <input type="checkbox" name="cbMask" id="cbMask" class="css-checkbox" />
                        <label for="cbMask" class="css-label radGroup1">
                            View password? <span id="masking">NO</span>
                        </label>
                </p>
                    <input type="submit" class="button wide" value="Sign In">
                </form>
            </div>
        <script>
    var form = document.getElementById("theForm");
        form.onsubmit = function()
        { 
            var errorQ = "some errors were found: \n \n";
            var same = errorQ;

            var userName = document.forms[0].userName.value;
            var lenUserName = userName.length;
            var pswd = document.forms[0].pswd.value;
            var lenPswd = pswd.length;

             if (userName == "" || userName == null || lenUserName <= 0){
                document.getElementById('usernameLogin').className = 'error';
                document.getElementById('name-error').innerHTML = "Username is required";
                document.getElementById('name-error').style.display = "block";  
                errorQ += "password cannot be empty \n";            
            }

            else if (userName != "" || lenUserName > 0){
            document.getElementById('usernameLogin').className = 'default';
            document.getElementById('name-error').innerHTML  = "";
            document.getElementById('name-error').style.display = "none";
            }

            if (pswd == "" || pswd == null || pswd <= 0){
                document.getElementById('passwordLogin').className = 'error';
                document.getElementById('pswd-error').innerHTML = "Password is required";
                document.getElementById('pswd-error').style.display = "block";
                errorQ += "password cannot be empty \n";
            }
            else if (pswd != "" || lenPswd.length > 0){
                document.getElementById('passwordLogin').className = 'default';
                document.getElementById('pswd-error').innerHTML = "";
                document.getElementById('pswd-error').style.display = "none";
                }

        if (errorQ == same){
            return true; 
        }   
        else  { 
            //alert("start over " + errorQ);    
            return false;
            }
       return false;
       }

       form.onchange = function() {
           var errorQ = "some errors were found: \n \n";
           var same = errorQ;

           var userName = document.forms[0].userName.value;
           var lenUserName = userName.length;
           var pswd = document.forms[0].pswd.value;
           var lenPswd = pswd.length;

        if (userName == "" || userName == null || lenUserName <= 0){
            document.getElementById('usernameLogin').className = 'error';
            document.getElementById('name-error').innerHTML = "Username is required";
            document.getElementById('name-error').style.display = "block";  
            errorQ += "password cannot be empty \n";            
        }

        else if (userName != "" || lenUserName > 0){
        document.getElementById('usernameLogin').className = 'default';
            document.getElementById('name-error').innerHTML  = "";
            document.getElementById('name-error').style.display = "none";
        }

            if (pswd == "" || pswd == null || pswd <= 0){
                document.getElementById('passwordLogin').className = 'error';
                document.getElementById('pswd-error').innerHTML = "Password is required";
                document.getElementById('pswd-error').style.display = "block";
                errorQ += "password cannot be empty \n";
            }
            else if (pswd != "" || lenPswd.length > 0){
                document.getElementById('passwordLogin').className = 'default';
                document.getElementById('pswd-error').innerHTML = "";
                document.getElementById('pswd-error').style.display = "none";
                }       

            if (errorQ == same){
                return true; 
            }   
                else  { 
                //alert("start over " + errorQ);    
                return false;
           }
        return false;
       }
    </script>
    </body>
    </html>

Ben · Accepted Answer · 2014-09-29 20:28:03Z

Sure. I think one piece of advice I often give to people is to always use readable variable names. It's not your job to compress the variable names for a smaller script download, a minifier can do that. So name things like password instead of pswd, for future maintainability.

You can simplify your 'empty' checking by testing that the field has a length property:

// Too many checks that all do the same thing, hard to read
if (userName == "" || userName == null || lenUserName <= 0) {

// One check, simple and readable
if (!userName.length) {

In fact, you have cached the length in a variable before, so let's use it. Note that it's good practice to put the positive conditions before the negative ones.

if (lenUserName) {
    // everything is OK
} else if (!lenUserName) {
    // validation failed
}

I would also consider renaming this to something a little more explicit, such as usernameLength or userNameLength.

You can save yourself writing return true and return false by returning the boolean value of an expression:

// Too long winded
if (errorQ == same) {
    return true; 
} else {    
    return false;
}

// Can be one line - and you should use strict equality checking!
return errorQ === same;

You should look at caching the result of DOM lookups, for a slight performance benefit, less code download and error prevention (repeating the same code again could lead to typos).

if (pswd == "" || pswd == null || pswd <= 0) {
    document.getElementById('passwordLogin').className = 'error';
    document.getElementById('pswd-error').innerHTML = "Password is required";
    document.getElementById('pswd-error').style.display = "block";
    errorQ += "password cannot be empty \n";
} else if (pswd != "" || lenPswd.length > 0) {
    document.getElementById('passwordLogin').className = 'default';
    document.getElementById('pswd-error').innerHTML = "";
    document.getElementById('pswd-error').style.display = "none";
}

// After utilising caching, etc

var passwordLogin = document.getElementById('passwordLogin'),
    passwordError = document.getElementById('pswd-error');

if (lenPswd) {
    passwordLogin.className = 'default';
    passwordError.innerHTML = "";
    passwordError.style.display = "none";
} else if (!lenPswd) {
    passwordLogin.className = 'error';
    passwordError.innerHTML = "Password is required";
    passwordError.style.display = "block";
    errorQ += "password cannot be empty \n";
}

I think you have also repeated yourself by creating an anonymous function for both the onsubmit and onchange handlers with the same contents. You should do something like the following:

var formValidation = function() {
    // your custom code
};

var form = document.getElementById("theForm");

form.onsubmit = formValidation;
form.onchange = formValidation;

Finally, you should look into abstracting your required field checking into a method itself, saving you from having to duplicate that logic for each field that you were to add to the form. Such an implementation could take this form:

var validateField = function(value, element, error, message) {
    if (value.length) {
        element.className = 'default';
        error.innerHTML = "";
        error.style.display = "none";
    } else {
        element.className = 'error';
        error.innerHTML = message;
        error.style.display = "block";
        errorQ += message + ' \n';
    }
};

// Example for the password field
validateField(
    document.forms[0].pswd.value, 
    document.getElementById('passwordLogin'),
    document.getElementById('pswd-error'),
    'Password is required'
);

Hope this helps.

Thank you, Ben! I really liked how you put all the functional code in "var formValidation = function()," and then used different event handlers to run the function! Nice. I just learned a ton by that. thnx! I didn't understand the last code example as much. What do you call that structure? Also, I used the "errorQ += message + ' \n';" to run a comparison test because of my limited knowledge of catching errors and stopping the function (i.e., return false). Is there a better way, so I can eliminate that part? — ryanjohnsond, Sep 29 '14 at 21:30
There's a number of possibilities that you can go with the last example, but the general gist was to have that validateField function inside the formValidation function - then you can call it twice for your name and password fields, whilst still referencing the errorQ var, i.e. var formValidation = function() { var errorQ = ''; var same = errorQ; var validateField = function(value, element, error, message) { // function body }; validateField(user); validateField(pass); return errorQ === same; }. This validateField pattern then can be reused for other forms down the line. — Ben, Sep 29 '14 at 22:45
Ben, I am having trouble mapping the parameters you gave in my head. What does the arguments "value, element, error, message" mean? One is a name.value, one is an ID, Class and string? Could you explain how JS knows which params are used for what? — ryanjohnsond, Oct 3 '14 at 20:03
Naming is really up to you on this one. If you have a look at the method call, it shows you; the first argument is the value of the name/password field, the second is the name/password element itself, the third is your error element, for your validation messages, and the last one is the message that is shown when the validation failed. If these aren't descriptive enough for you, you could probably lengthen the variable names. e.g. message could be failedValidationMessage, if you think its more readable. :-) — Ben, Oct 3 '14 at 23:44

cimmanon · Accepted Answer · 2014-09-29 21:04:09Z

You have a lot of hard coded values that make it quite difficult to reuse for another project (eg. var userName = document.forms[0].userName.value). What if you want to have your form fields named differently? Or in a different order? What if your form isn't the first form on the page?

If you ever get the feeling like you're copy/pasting code, then you need to start breaking it out into little reusable pieces. You've got the same code duplicated in your onsubmit and onchange methods. You could be pointing both methods to the same function.

form.onsubmit = checkIt;
form.onchange = checkIt;

function checkit() {
    // do stuff
}

The modern method of form validation is via HTML5 form attributes:

<input type="text" name="username" required />
<input type="password" name="password" required />

But since there are still a few browsers out there that don't support them (IE < 9 or 10), you still want to have JavaScript fallback. Because the validation requirements are part of the DOM, you don't have to rely on positioning or specific names to decide on what the requirements are for each field.

form.addEventListener('blur', validateField, true);

function validateField(ev) {
  // There's probably a better way of doing this check...
  var isRequired = typeof ev.target.getAttribute('required') === 'string';

  if (isRequired && ev.target.value.length == 0) {
    console.log('The ' + ev.target.name + ' is required');
  }
}

http://codepen.io/cimmanon/pen/wCGvo

Open up your console. console.log() is a less annoying way to debug than alert(). — cimmanon, Sep 30 '14 at 17:49
Don't know how to do the console.log(), but I should look into that. Thanks for showing how to point both methods to the same function. That's very helpful. — ryanjohnsond, Sep 30 '14 at 21:50

Jason Crosby · Accepted Answer · 2014-09-30 12:56:40Z

1

A few things I noticed.

1. Separate JavaScript and CSS out of your HTML.
2. You appear to be executing the exact same code in onchange and onsubmit. Pull that code out to a separate function and just call that function inside onchange and onsubmit.
3. You can replace if (errorQ == same){ return true; } else { //alert("start over " + errorQ); return false; } return false;

with return errorQ == same;
4. Not sure how the code looks like in your IDE but if it looks like it does here please fix your indentations. Code like this is really hard to read.

OPTIONAL
1. There are a few input tags that are not closed <input type="submit" class="button wide" value="Sign In">. Make sure you close them using /> in place of > at the end. This is not required but makes it easier to read the code

edited Sep 30 '14 at 12:56

answered Sep 29 '14 at 20:28

Jason Crosby

1213 bronze badges

1

\$\begingroup\$ Using <input /> and <input /> are both valid in HTML5, <input /> is only required for XHTML. \$\endgroup\$ – cimmanon Sep 29 '14 at 20:47
\$\begingroup\$ You can replace the entire piece of code on point 4 with "return errorQ === same;"? \$\endgroup\$ – ryanjohnsond Sep 30 '14 at 2:11
\$\begingroup\$ @ryanjohnsond yes you can. The first if check states if errorQ == same is true than return true else return false. Which is the same as return errorQ == same. The last return false is not needed as the result is the same as when errorQ == same is false. \$\endgroup\$ – Jason Crosby Sep 30 '14 at 12:51
\$\begingroup\$ @cimmanon I did not know that thanks for the info. However it still would make for easier to read code as the reader would easily be able to tell where the input was terminated. I have edited my answer accordingly. \$\endgroup\$ – Jason Crosby Sep 30 '14 at 12:55

Add a comment |

Stack Exchange Network

current community

your communities

more stack exchange communities

Form validation appears to be too rigid

3 Answers 3

Your Answer

Not the answer you're looking for? Browse other questions tagged javascript css validation or ask your own question.

Hot Network Questions

Form validation appears to be too rigid

3 Answers 3

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged javascript css validation or ask your own question.

Related

Hot Network Questions