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 have written some jQuery code for AJAX form submission:

jQuery(function() {
    jQuery('#message').click(function() {
        var btn = jQuery(this)
        var Name = jQuery("#Name").val();
        var Email = jQuery("#Email").val();
        var Sub = jQuery("#Sub").val();
        var Message = jQuery("#Message").val();
        if (Name == "") {
            jQuery('#Name').focus();
            jQuery("#Name").after('<span class="fa fa-exclamation-triangle form-control-feedback"></span>');
        }
        else if (Email == "") {
            jQuery("#Email").focus();
            jQuery("#Email").after('<span class="fa fa-exclamation-triangle form-control-feedback"></span>');
        }
        else if (Sub == "") {
            jQuery("#Sub").focus();
            jQuery("#Sub").after('<span class="fa fa-exclamation-triangle form-control-feedback"></span>');
        }
        else if (Message == "") {
            jQuery("#Message").focus();
            jQuery("#Message").after('<span class="fa fa-exclamation-triangle form-control-feedback"></span>');
        }
        else if (!validateEmail(Email)) {
            jQuery("#Email").focus();
        }
        else {
            btn.button('loading')
            $.ajax({
                url: '/application',
                dataType: 'text',
                type: 'post',
                data: {querytype: 'form', name: Name, email: Email, sub: Sub, message: Message},
                success: function(data)
                {
                    jQuery('#success').fadeIn();
                }
            });
        }
    });
});

Is this code good to go? Or it can be improved? I have seen this where they check the validation after form submission, or this.

Should I check validation before or after submission?

Will my approach be good if I need to handle 10+ input elements?

share|improve this question

2 Answers 2

up vote 17 down vote accepted

A few things:

  • Variables in JavaScript should be camelCased. PascalCase is traditionally left for types.
  • CSS classes are generally lower-case and hyphenated. Instead of #Name, consider #name (and the corresponding HTML attribute, id='name')
  • Use $ instead of jQuery. $ is syntactic sugar that essentially resolves the same thing, but it's one character instead of 6, and it's more conventionally used (see below)
  • Consider using the jquery.validate library. This will make your validation significantly less painful. If you're using ASP MVC you can use jquery.validate.unobtrusive to automatically map your validation on your view model from C# to JavaScript.
  • Keep your semi-colon conventions consistent. Use them at the end of every statement, or don't use them at all.
  • If you're going to use $('#element') calls a lot, consider calling that $('#element') once and storing it inside of a variable inside the scope you are using that element.

With regards to validation, you should always perform validation on the server side. I cannot overstate that point. You must do it. Client-side validation is a bonus that should be performed in addition to server-side validation, but should not be relied upon and you should not make it core functionality to your site (users can easily disable JavaScript, and some browsers like Tor have JavaScript disabled by default).

Remember that the golden rule in programming is that end-users are stupid. The weakest point to any application is the point that the end-user is allowed to provide input. You should treat the data received from the form as such, even if you already have client-side validation.

Your approach will work with 10+ input elements, but you will be repeating a lot of code. As suggested above it's advisable for you to check out jquery.validate which will allow you to maintain validation rules a lot easier (and DRY up your validation)

I don't think you should be concerned whether or not your code is professional but rather that it signals it's intent to other developers clearly and it is maintainable. Your code is certainly clear in what it tries to do it but strikes me as scaling very badly in terms of maintainability, and I think you should address that.

Example of jQuery Validate

Well, I've only really used jquery.validate.unobtrusive, but here's a link to the library. I'm sorry I can't provide much more info on that, as most of it was done automatically for me behind the scenes by ASP MVC 4.

jQuery in WordPress

Wordpress automatically no-conflicts jQuery as a matter of courtesy. If you're only using jQuery, you can override this and just put in a $ = jQuery; somewhere. Better yet, you can use jQuery(function($) { ... to alias $ to jQuery inside your ready handler. – Schism

Or, as pointed out by @Pinoniq in his answer, you can wrap it in an IIFE (aka self-executing functions).

share|improve this answer
    
HI Thanks for the info. Currently I am just sending this data without the form element. So should I use HTML form? Can you give a good example of jquery.validate. I know about $, but I am using this as in wordpress and it does not let me allow $. –  Sameer Sam Sep 3 at 10:19
    
Semantically speaking, any information sent from a HTML client should be sent through a POST action. The easiest way of doing this is a <form>, so yes, I would say you should use a form. How does wordpress influence your choice to use $? JavaScript is executed purely client-side (note: I haven't looked into using JavaScript w/ wordpress yet so I might be wrong). Note that jquery.validate requires that you use a <form> –  Dan Pantry Sep 3 at 10:21
    
Wordpress gives error like " .. is not a function ". Okay, so I need to use POST. But if I send as it is without any post, is that not safe? –  Sameer Sam Sep 3 at 10:25
    
According to you, I think this is what I should work scotch.io/tutorials/javascript/… –  Sameer Sam Sep 3 at 10:26
1  
@SameerSam Wordpress automatically no-conflicts jQuery as a matter of courtesy. If you're only using jQuery, you can override this and just put in a $ = jQuery; somewhere. Better yet, you can use jQuery(function($) { ... to alias $ to jQuery inside your ready handler. –  Schism Sep 3 at 14:18

Code should read like poetry

People who don't know that jQuery(function); is the same as jQuery(document).ready(function); will have a hard time understanding this code.

For better readability start your variable names with a $ if it is a jquery object.:

var $btn = jQuery(this);

Use descriptive variable names. I really hate all those btn, frm, sub, .. abbreviations. It's a button. so call it a button:

var $button = $(this);

As already pointed out by Dan, we use camelCase for variable names in JavaScript (and css/html):

var name = $("#name").val();

Comments

I don't see any comments. so it is really hard to scan your code. There is no white space, no new lines. Just all code. this makes it harder to read. I don't want you to go all crazy on the comments. A simple

$('#message').click(function() {

    //validate form inputs
    ...

    //send data to server
    ...

});

is enough.

DRY

Instead of first looking up a DOM element and getting it value, and then later on looking that same element up again. Lookup the element and get the value when you need it. this also keeps your easier to maintain. If your form element name changes, you only need to change your code at one place.

Professional code

Let's define 'professional code' as code that is easy to use and needs as little editing as possible. Yours isn't.

If the HTML form changes. your JS changes. If you add a form element, your JS has to change.

The design

What you did was code 2 completely different things into one. Validation and Ajax. Apart from that, your validation isn't really user-friendly. If you have an empty name, and a wrong email. You first get an error sign at the name, then you try to resubmit. and now it argues about the email. UX-nightmares

Validation

Validation on server side is there mostly to stop the scriptkiddies from hacking you.

Validation on the client side exists ass some extra sugar to add instant validation. The best sugar here is an as-you-type validation. This will help the non-it-nerds filling in only digits as a phone number.

Ajax

Ajax is simple dumb sending of data to the server, and possibly do stuff when data is returned or the request completed.

Change the design

Your code should in fact be split up into two different pieces. One that send the form data on form submit. And one piece of code that adds some validation sugar. This validation could happen on the click of a submit button or when an input element loses focus or even an as-you-type validation.

To sum up

$(document).ready(function() {
    //all elements we need
    var $form = jQuery('#my-form'),
        $message = jQuery('#message'),
        $name = jQuery("#name"),
        $email = jQuery("#email"),
        $sub = jQuery("#sub");

    //some extra functions
    var showSuccess = function() {
        $success.fadeIn();
    }

    function showError(element) {
        element.after('<span class="fa fa-exclamation-triangle form-control-feedback"></span>');
    }

    //add some validation
    $message.on('click', function() {
        if (!$name.val()) {
            showError($name);
        }

        if (!($email.val() && validateEmail(Email))) {
            showError($email);
        }

        if (!$sub.val()) {
            showError($sub);
        }

        if (!$message.val()) {
            showError($message);
        }
    });

    //send form with ajax
    $form.on('submit', function() {
        $message.button('loading');

        $.ajax({
            url: '/application',
            dataType: 'text',
            type: 'post',
            data: $(this).serialize(),
            success: showSuccess
        });
    });
});

Things I changed:

  • on('click') instead of click(). this is personally, I think that on(); reads better. You are acting on a click event, not clicking it.
  • Removed the =="" comparisons. Simply let JS cast it to a boolean for you.
  • moved the success-callback function out of the $.ajax() call. Makes it easier to read and decouples it a little.

Improvements should be done by you: Instead of having all those if (!$element.val()) checks, this should be decoupled from your actual form elements. You could use a jQuery validation plugin. Or you could loop over the elements:

$message.on('click', function() {
    $.each(toValidate, function(key, $element) {
        if ($element.val()) {
            showError($name);
        }
    });
});

with

var toValidate = [$name, $email, $sub, $message];
share|improve this answer
    
As addressed in my answer, OP cannot use $ due to incompatibilities with Wordpress' JavaScript libaries –  Dan Pantry Sep 3 at 10:56
3  
Then wrap it in an IIFE –  Pinoniq Sep 3 at 10:57
    
I can't agree with your suggestion of comments, except maybe the else that handles submission. Everything else is immediately evident, although it definitely would benefit from whitespace. +1 for a great write-up though! –  Schism Sep 3 at 14:20
    
@Schism why don't you agree on comments? –  Pinoniq Sep 3 at 15:04
1  
@Pinoniq Your first comment for example is off the mark, jQuery(function) is the one true way. I think @Schism might have exaggerated though ;) –  konijn Sep 3 at 15:13

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.