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];