Can people understand my form validation code?

Question

I've just finished creating this form validation but I want to make it public for beginner 'contact us forms'.

I was wondering if I can have some peoples' input on if they can understand/read my JavaScript. Although I know my JS works, it's better to be safe than to have 20 people asking what this does. It's just input feedback.

What I'm trying to achieve in my code is making fields required with JavaScript.

The following fields are REQUIRED:

• First Name
• Last Name
• Email
• Phone

Most of the functions such as alphaNumeric only allows numbers and onlyAlphameric only allows letters.

E.g for each function or what not, add a comment saying what it does.

For example, adding a comment:

This function does this and triggers that and so on.

JavaScript:

function elem(id) {
    return document.getElementById(id);
};


window.onload = function () {
    document.querySelector("#RadioGroup1_0").click();


    var form = document.getElementById('form');
    form.onsubmit = function (e) {
        var rules = [
        ['first-name', elem('first-name').value.length > 0],
        ['last-name', elem('last-name').value.length > 0],
        ['email', elem('email').value.length > 0 && /^([a-zA-Z0-9_\.\-])+\@(([a-zA-Z0-9\-])+\.)+([a-zA-Z0-9]{2,4})+$/.test(elem('email').value)],
        ['phone', elem('phone').value.length > 7 && elem('phone').value.length < 11 && /^(\+\d{1,2})?[\d ()-]+$/.test(elem('phone').value)]

    ];

    function myFunction()
{
document.getElementById("myForm").reset();
}

        function alpha(e) {
            var k;
            document.all ? k = e.keyCode : k = e.which;
            return ((k > 64 && k < 91) || (k > 96 && k < 123) || k == 8);
        }

        var valid = true;
        var firstFocus = null;
        for (var i = 0; i < rules.length; i++) {
            var parent = elem(rules[i][0]).parentNode;
            if (!rules[i][1]) {
                valid = false;
                parent.children[2].style.display = "inline";
                if (firstFocus == null) firstFocus = parent.children[1];
            } else {
                parent.children[2].style.display = "none";
            }

        }
          if (!valid) {
            firstFocus.focus();
            return false;
        }

        return true;
    };
};


function onlyAlphabets(e, t) {
    try {
        if (window.event) {
            var charCode = window.event.keyCode;
        } else if (e) {
            var charCode = e.which;
        } else {
            return true;
        }
        if ((charCode > 64 && charCode < 91) || (charCode > 96 && charCode < 123) || (charCode === 8)) return true;
        else return false;
    } catch (err) {
        alert(err.Description);
    }
}


var specialKeys = new Array();
specialKeys.push(8); //Backspace
function IsNumeric(e) {
    var keyCode = e.which ? e.which : e.keyCode
    var ret = ((keyCode >= 48 && keyCode <= 57) || specialKeys.indexOf(keyCode) != -1);
    //document.getElementById("phone").style.display = ret ? "none" : "inline";
    return ret;
}

Answer 1

Can people understand my code?

Its certainly not bad code - but I believe there are some things you can do which will improve the speed with which people like me can grasp what it is doing (and why).

Comments

Some people think well-written code needs no comments. I don't fall into that camp. Looking at your code I see the following:

Total number of comments is 2

Total number of helpful comments is 1

Total number of comments describing goal of sections of code is 0

Languages like Java have JavaDoc, which are comments in the code from which you can automatically produce documentation. If you've ever tried to re-use code that has no documentation you'll know how useful it is. If you've ever had to maintain documentation that is separate from code you'll know what a pain that is. Innaccurate documentation is worse than none.

So I believe it is best to put documentation into code. Ideally in a form from which documentation can be produced.

I like to treat every function/method I write as if it were one I would put into a library and re-use. To me this means it needs comments that document its purpose, interface and any quirks.

Magic Numbers

You have a lot of magic numbers, e.g. 48 and 57. These should be either

• explained in comments
• replaced by constants with meaningful names (e.g. ASCII_0, DIGIT_0 ...)
• replaced by functions

The latter may be best, especially if you (or anyone else) want this code to work with character sets other than ASCII.

Meaningful names

 function myFunction()

I feel you may be underestimating the usefulness of meaningful names as an aid to other's understanding. I'd try to find a more helpful name that helps others understand the purpose of the the function and what it returns.

Writing for comprehension

You use the ternary operator. Often this is used for assignment. If you use if for assignment but in an unusual way you may momentarily confuse readers.

Unless you need conciseness, it can sometimes be clearer to use a conventional if-then-else form instead.

Wouldn't

document.all ? k = e.keyCode : k = e.which;

be clearer as

k = document.all ? e.keyCode : e.which;

Because the main purpose seems to be to assign a value to k. The earlier style is one I might use if the main point is to call a function and, as an afterthought, evaluate its's success or failure. I think in this case it moves the reader's focus to the wrong place.

Writing portable code and avoiding browser-sniffing

The above use of the ternary operator can probably be avoided entirely. I suspect you may be interested in jQuery and replace it with

keyPressed = e.which

Which I find easier to read and which can be understood more rapidly.

I would at least factor out browser-dependent and/or repeated stuff into my own functions/methods and/or carefully document in comments what it is doing and why it needs to be done.

---

Documenting purpose

What I'm trying to achieve in my code is ...

As I mentioned above, I believe it is very useful to have this described in comments in the code itself (others may disagree). From it's inclusion in your question you evidently believe this helps readers understand the following code. Therefore I'd put it into the code as a comment.

Answer 2

Take care that I can read JavaScript but it was a whole time ago that I effectively wrote in that.

1. I see you've created your own isNumeric function.

   Maybe you can use the isNaN. Documentation over here.

2. You use this twice:

   ((k > 64 && k < 91) || (k > 96 && k < 123) || k == 8);
   

   If you use such a statement more than once, you should consider refactoring to a method.

3. What is the meaning of t? (you don't use it)

   function onlyAlphabets(e, t) {
       try {
           if (window.event) {
               var charCode = window.event.keyCode;
           } else if (e) {
               var charCode = e.which;
           } else {
               return true;
           }
           if ((charCode > 64 && charCode < 91) || (charCode > 96 && charCode < 123) || (charCode === 8)) return true;
           else return false;
       } catch (err) {
           alert(err.Description);
       }
   }
   

4. Very bad naming of method:

   function alpha(e) {
       var k;
       document.all ? k = e.keyCode : k = e.which;
       return ((k > 64 && k < 91) || (k > 96 && k < 123) || k == 8);
   }
   

   Don't you do it almost the same as the function mentioned in the third issue?

Answer 3

In my eyes, any function called myFunction is not a good function.

This applies to foo() and helloWorld() too.

Having another look at the code, I can see that it involves a form and resetting values.

Maybe a function as follows would be better:

function formReset() { }

In an nutshell a function name in my view should be

1. relatively short
2. straight to the point

For instance:

function addOrder() {}

looks much better in my view than

function addOrderToShoppingCart()

Both are pretty obvious, one just takes a little bit longer to read.

Answer 4

I will try not to repeat items that others have mentioned, since I agree with all of them.

Here is what I have found:

1. Not using helper code that you have written

   At the top you define a function:

   function elem(id) {
       return document.getElementById(id);
   };
   

   but then in two other places, I see these:

   var form = document.getElementById('form');
   

   and

   function myFunction()
   {
       document.getElementById("myForm").reset();
   }
   

   Why not use your elem function since you appear to have created it as a short form, so you could use it as:

   var form = elem('form');
   

   for the first example above and

   function myFunction()
   {
       elem('myForm').reset();
   }
   

   Note: I agree with the above posters about poor choice of naming functions.

2. Standardize on quotes

   Try pick a standard set of quotes to use, either single quotes everywhere, or double quotes everywhere and stick to one of those styles.

3. Dangers of chaining

   Again on the topic of document.getElementById(), I always try to check if the value returned is a null prior to assuming that properties exist on that object.

   e.g.

   var myTextBox = document.getElementById('my-textbox');
   if (myTextBox) {
       return myTextBox.value.length;
   }
   

   rather than:

   return document.getElementById('my-textbox').value.length;
   

Answer 5

Your email checker is broken. You fail to allow various characters that are legal in the local part (such as +) and you fail to allow TLDs longer than four characters, of which there are now many.

Also, you myFunction, alpha, onlyAlphabets, and IsNumeric functions are never called. It would help to see how these are being used to determine if they are being used correctly.

Answer 6

You should read more about how JavaScript works, especially variable hoisting.

if (window.event) {
    var charCode = window.event.keyCode;
} else if (e) {
    var charCode = e.which;
}

Here you are declaring two variables with the name charCode. But you want to declare it only once and then initialize it in your if-block.

var charCode;
if (window.event) {
    charCode = window.event.keyCode;
} else if (e) {
    charCode = e.which;
}

Generally put all your var statements directly beneath your function keyword

function calculateSomething() {
    var a, b, c;
    ...
}

Also read about the comparison operator and use the strict === instead of == (and !== instead of != resp.)