current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

I made a simple form validation. It's working alright. The thing is I want my code to be optimized. Can somebody show me some pointers to improve my code? Thanks in advance.

$(document).ready(function(){

$('.submit').click(function(){
     validateForm();     
});


function validateForm(){
    var nameReg = /^[A-Za-z]+$/;
    var numberReg =  /^[0-9]+$/;
    var emailReg = /^([\w-\.]+@([\w-]+\.)+[\w-]{2,4})?$/;

    var names = $('#nameInput').val();
    var company = $('#companyInput').val();
    var email = $('#emailInput').val();
    var telephone = $('#telInput').val();
    var message = $('#messageInput').val();

    var inputVal = new Array(names, company, email, telephone, message);

    var inputMessage = ["name", "company", "email address", "telephone number", "message"];

    var textId = ["#nameLabel", "#companyLabel", "#emailLabel", "#telephoneLabel", "#messageInput"];

     $('.error').hide();

        if($.trim(inputVal[0]) === ""){
            $('#nameLabel').after('<span class="error"> Please enter your ' + inputMessage[0] + '</span>');

        }else if(!nameReg.test(names)){
            $('#nameLabel').after('<span class="error"> Letters only</span>');
        }

        if($.trim(inputVal[1]) === ""){
            $('#companyLabel').after('<span class="error"> Please enter your ' + inputMessage[1] + '</span>');
        }

        if($.trim(inputVal[2]) === ""){
            $('#emailLabel').after('<span class="error"> Please enter your ' + inputMessage[2] + '</span>');

        }else if(!emailReg.test(email)){
            $('#emailLabel').after('<span class="error"> Please enter a valid email address</span>');
        }

        if($.trim(inputVal[3]) === ""){
            $('#telephoneLabel').after('<span class="error"> Please enter your ' + inputMessage[3] + '</span>');

        }else if(!numberReg.test(telephone)){
            $('#telephoneLabel').after('<span class="error"> Numbers only</span>');
        }

        if($.trim(inputVal[4]) === ""){
            $('#messageLabel').after('<span class="error"> Please enter your ' + inputMessage[4] + '</span>');
        }   



    return false;

}   



});
share|improve this question

2 Answers 2

up vote 0 down vote accepted

I would use a for loop to test all values first. If one is invalid you can stop and fix before going on with the code or submitting. Here is a fiddle without the submit callback so you can see how this would work.

The following code is explained in the comments.

(function($) { //Wrap your function in an IIFE. Assign "$" to jQuery to avoid conflicts with other libraries.
    $('#myform').submit(function() { //Form can be submitted without a "click"
        //You only need to say "var" once. After that just use "," and bump to the next one.
        var names = "       ", //I set this one empty to show you how it works
            company = "           good",
            email = "dat shi cra";

        var inputVal = [names, company, email],
            inputMessage = ["name", "company", "email address"],
            textId = ["#nameLabel", "#companyLabel", "#emailLabel"];


        for(var i=0;i<inputVal.length;i++){
            //We want to trim before the testing.
            //$.trim doesn't cut white space within the string.
            //ie: "     good" will return "good", but "goo    d" will return "goo    d".
            inputVal[i] = $.trim(inputVal[i]);

            //Here is where we test for invalid entries.
            //Is the value undefined? Is it null? Or is it an empty string? If any of these, it is invalid.
            //You can add more tests if you like.
            if (typeof inputVal[i] === 'undefined' || inputVal[i] === null || inputVal[i] === "") {
                // "i" will tell you which position is invalid.
                // Call the function and tell it which one is invalid.
                invalidEntry(i);

                //Stop at the first invalid we find.
                //If you want to collect all the invalids, remove the return, make an array, and push the "i" to it, then do something with the array.
                //When the loop is done that array will contain all the invalid entries numbered accordingly.
                return false;
            }
            //This will run for EACH VALID entry.
            console.log("Yolo bitcholo! So far so good!");
        };

        function invalidEntry(i) {
            //Get the index of the invalid and do your stuff.
            $(textId[i]).after("<span class='error'>Please enter a valid " + inputMessage[i] + ".</span>");
        }

        return this.some_variable //Only submit if this variable is set. This can contain your form data, which we would only want to submit if there is data to submit.
    });
})(jQuery);
share|improve this answer
up vote 1 down vote

  • First of all, you shouldn't use $('.submit').click() but $('#myForm').submit() as you will be able to submit forms by pressing enter too.

  • To check if a string is a number, use !isNan(string) (see this answer for more information).

  • Email regex are difficult things. Just check if there is someting before the @ (with .+?) and if there is something after the @ (again with .+?) and if that last part contains a . and a couple of characters (from 2 to 6)

  • It would make the function a lot easier if you put all specific code outside it. E.g:

    validateForm({
        '#nameInput': {
            'valid': function (value) {
                return /^[a-z]+$/i.test(value);
            },
            'label': '#nameLabel',
            'messages': {
                'empty': 'Please enter your name',
                'invalid': 'Letters only',
            },
        },
        '#companyInput': {
            'label': '#companyLabel',
            'messages': {
                'empty': 'Please enter your company',
            },
        },
        // ...
    });
share|improve this answer

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.