3
\$\begingroup\$

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].' - ';
    }

?>
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ +1 I like this Nice clean readable code. The only thing I would change is i and ii. I'm one for explicit names so I would go for indx and total or maybe currentIndex and requiredLength or... etc. etc. You get me point. Not that it is a big deal as it is still readable and easy to understand. \$\endgroup\$ – James Khoury Aug 24 '11 at 0:19
  • \$\begingroup\$ @JamesKhoury We have a coding practice where i is a for loop counter and ii is the for loop length. It's quite common. \$\endgroup\$ – Raynos Aug 24 '11 at 9:16
  • \$\begingroup\$ I've seen it once or twice before but I didn't realise it was a style. I'm personally not a fan of it but each to his own then ;) \$\endgroup\$ – James Khoury Aug 24 '11 at 23:58
1
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ +1 the definition, implementation, and instantiation being separated makes my C# dev toes tingle. I think all javascript should be written with the 3 all separated \$\endgroup\$ – Jimmy Hoffa Aug 23 '11 at 12:38
  • \$\begingroup\$ Ugh, the definition & implementation are the same thing in JavaScript. We only have objects. \$\endgroup\$ – Raynos Aug 23 '11 at 12:47
  • 1
    \$\begingroup\$ Naming conventions like FormValidator$validate are going to become overkill and a pain. Those get out of control quickly. It also only pollutes the namespace in legacy versions of IE. Also please don't recommend Singletons like that. Just use object literals. \$\endgroup\$ – Raynos Aug 23 '11 at 13:16
  • 1
    \$\begingroup\$ @Raynos I was referring to the prototype as the definition, and using the word implementation in the sense of an object inheriting another implements it. I realize prototypes are not the same thing as inheritance, but it's the closest you get with javascript. \$\endgroup\$ – Jimmy Hoffa Aug 24 '11 at 12:41
  • 1
    \$\begingroup\$ @Raynos I've changed the naming. I wasn't suggesting a naming convention as much as I was suggesting that it be named. seeing "Anonymous function" a lot in a call stack really doesn't help. \$\endgroup\$ – James Khoury Aug 25 '11 at 0:00
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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