Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
1  
Welcome to Code Review! Asking us to provide the comments about what your code does makes me wonder: Do you know what your code does? We'd gladly review the readability of your code, but we do assume that you understand your own code. Because, this is your code, right? –  Simon André Forsberg May 8 at 11:27
    
To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question –  Simon André Forsberg May 8 at 11:29
    
@SimonAndréForsberg Hi there, yes this is my code that I wrote, I originally posted it at another section of StackOverflow so they just redirected me here and I had to create a new account, or 'Post as a guest'. –  Salata May 8 at 11:30
    
Good. Thanks for editing your question and providing a short description. Again, Welcome to Code Review! –  Simon André Forsberg May 8 at 11:42
    
@SimonAndréForsberg Thanks for the helping had bud, if you have time do you reckon you could go over my code? Cheers –  Salata May 8 at 11:43

6 Answers 6

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.

share|improve this answer
    
Welcome to CodeReview, feel free to drop by in our Code Review Chat –  Vogel612 May 8 at 14:59
2  
Some people think well-written code needs no comments. I think this. Self-documenting code is the way to go. Every now and then you run into situation where you have to do something a little unclear, which is where you leave comments. If there's too many comments in code, it's as good as none, I don't know which ones are useful so read none. If you code DEPENDING on comments to explain it, then there's definitely something wrong. –  Cruncher May 8 at 16:06
    
@Cruncher: I agree with most of that. But not all :-). However the subject deserves it's own question and answers - for example this. So I'll leave it at that for now. –  RedGrittyBrick May 8 at 16:18
    
I disagree... some coders are better than others. What an excellent experienced coder might see simple and straightforward and lesser experienced coder might struggle with. Obviously never add pages and pages of code comments but I always find something in relation to comments is better than none at all. –  Zabs May 8 at 16:32
1  
To comment or not to comment — that has been debated on Programmers to the point where the question had to be locked, so it's not going to get resolved in this comment thread. –  200_success May 8 at 22:35

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?

share|improve this answer
    
I would add on the third point that the if(expression) return true else return false beginner code should be changed to return (expression). In this case, expression should probably be a function call...that's a long (somewhat) complicated expression. –  Casey May 8 at 15:45

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.

share|improve this answer
2  
addOrder is named with a verb-noun scheme, so naming formReset with a noun-verb scheme would be inconsistent. –  200_success May 8 at 16:53

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;
    
share|improve this answer

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.

share|improve this answer

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.)

share|improve this answer
    
Keep in mind, that changing the comparison operation will affect the behaviour. For a validator the strict comparison may be better, but in other cases I wouldn't recommend it ;) Either way, nice answer and welcome to Code Review. Maybe you want to check out our Code Review Chat too ;) –  Vogel612 May 9 at 6:44

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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