Validating a form and alerting the server if passed

Question

This is my first script in object-oriented JavaScript (jQuery 1.5.2). Its purpose is to simply validate a form (nothing fancy at the moment just checks if any value is present on required fields) and if validation is passed it submits the form via Ajax and alerts the server's response.

It would be great if more experienced programmers could review this and tell me what could be improved and what's dead wrong.

Here is the full working page:

<script type="text/javascript">
var formObj = {
     validate : function(theForm, required) {
            var toValidate = required.split(',');
            for (i in toValidate) {
                 var toValidateField = $('*[name*='+$.trim(toValidate[i])+']', theForm);
                 var valueLength = toValidateField.val().length;
                 if (valueLength === 0) {
                        toValidateField.addClass('invalid');
                        formObj.inputListener(toValidateField);
                 }
            }
            if ($('.invalid').length === 0) {
                 return 'valid';
            } else {
                 return 'invalid';
            }
     },
     inputListener : function(theField) {
            theField.keyup(function() {
                 if ($(this).val().length > 0) {
                        theField.removeClass('invalid');
                 } else {
                        theField.addClass('invalid');
                 }
            });
     },
     ajaxSubmit : function(formToSubmit) {
            $.ajax({
                 url: formToSubmit.attr('action'),
                 type: formToSubmit.attr('method'),
                 data: formToSubmit.serialize(),
                 success: function(output) {
                        //return output;
                        alert(output);
                 },
                 error: function() {
                        alert('Something went wrong!');
                 }
            }); 
     }

} // end formObj

</script>

<script type="text/javascript">
     $(function() {
            $('form').submit(function(e) {
                 e.preventDefault();
                 var thisForm = $(this);

                 // validate form
                 validate = formObj.validate(thisForm, 'username, message');

                 // if valid submit via ajax
                 if (validate === 'valid') {
                        formObj.ajaxSubmit(thisForm);
                 }
            });
     });
</script>

<style type="text/css">
     .invalid { border: 2px solid red; }
</style>

<form action="script.php" method="post">
     <label>Choose Username</label><br />
     <input type="text" name="username" /><br />
     <label>Choose Password</label><br />
     <input type="text" name="password" /><br />
     <label>Message to Admin</label><br />
     <textarea name="message"></textarea><br />
     <button type="submit">Submit</button>
</form>

Contents of script.php

<?php

    while (list($key, $value) = each($_POST)) {
        echo $_POST[$key].' - ';
    }

?>

Answer 1

The code:

http://jsfiddle.net/PZN2E/37/

The HTML:

<form action="script.php" method="post">
    <label>Choose Username</label><br />
    <input type="text" name="username" id="username" /><br />
    <label>Choose Password</label><br />
    <input type="text" name="password" id="password" /><br />
    <label>Message to Admin</label><br />
    <textarea name="message" id="message"></textarea><br />
    <button type="submit">Submit</button>
</form>

I've added "id" fields for all your inputs. This makes it easier to target them using JavaScript

The JS

(function($) {
    var inputListener = function _inputListener(field) {
        if (!field[0]._bound) {
            field[0]._bound = true;
            field.keyup(function _listener() {
                if (field.val().length > 0) {
                    field.removeClass('invalid');
                } else {
                    field.addClass('invalid');
                }
            });    
        }
    };

    $.fn.validate = function _validate(required) {
        var valid = true;
        for (var i = 0, ii = required.length; i < ii; i++) {
            var field = $("#" + required[i])
            if (field.val().length === 0) {
                field.addClass('invalid');
                valid = false;
                inputListener(field);
            }
        }
        return valid;   
    };
})(jQuery);

jQuery(function($) {
    $('form').submit(function(e) {
        e.preventDefault();
        var $this = $(this);

        // if valid submit via ajax
        if ($this.validate(['username', 'message'])) {
            $.ajax({
                url: $this.attr('action'),
                type: $this.attr('method'),
                data: $this.serialize(),
                success: function(output) {
                    // return output;
                    alert(output);
                },
                error: function() {
                    alert('Something went wrong!');
                }
            });
        }
    });
});

I've seperated your cod into two blocks. One is a closure to make avoid leaking global variables and the other is in a ready block.

The inputListener had no business being in an object so it's just a local variable, your ajax function also didn't have to be in an object if it's just being used once.

I've injected your validate function directly into $.fn to make it easier to use.

inputListener

This method will now check whether the function is bound to the field. Apart from that the code is the same.

validate

Using a flag to check whether it succeeded or failed (valid) is more optimum. I've changed the API so you pass in an array rather then a CSV string (ew!).

It's better to use a for loop, then a for ... in. For ... in on an array is simply inefficient and wrong.

Since we've added ids to the elements we can reference them by id directly.

your ready block

I've renamed theForm to $this as it's more readable.

Answer 2

Its only a small Quibble with your code but I find the naming of formObj a little odd. For me I see this as a FormValidator or FormExtender or some such.

Another point would be the for(i in ....) should be for(var i in ...) it will run either way and I believe it is just a matter of coding style.

Also you have functions like:

validate: function(theForm, required({....

I think it helps debugging if you do this instead:

validate: function FormValidate(....

It has the disadvantage of polluting the namespace but makes debugging easier as the function is now not anonymous.

If you were making this a class to instantiate you would write it like so:

var FormValidator = function(theForm)
{
    this.formElement = theForm;
    return this;
}

FormValidator.prototype = {

     validate : function(theForm, required) {
            //...
     },
     inputListener : function(theField) {
            //...
     },
     ajaxSubmit : function(formToSubmit) {
            //...
     }

};

var mainForm = new FormValidator(document.forms[0]);

If you were doing a Singleton pattern something like:

var FormValidator = function()
{
    return this;
}

function GetFormValidatorInstance()
{
    return window.__formValidatorInstance || new FormValidator();
}

These last two are probably overkill if you just want a simple Component. A more in depth example of the above can be found at https://codereview.stackexchange.com/questions/1658/javascript-code-class-structure

Answer 3

I'd like to complement James Khoury's answer:

Another point would be the for(i in ....) should be for(var i in ...) it will run either way and I believe it is just a matter of coding style.

The for(var i in ...) should be used instead of the other one; the for(i in ....) means you are declaring a global variable named i. Whenever you use a variable in javascript you haven't previously declared with var a new one will be created and it'll be a global one. If you execute the example code, a variable named i will be created with the last value it was attributed after the for loop.