jQuery AJAX form using bad practice

I have not worked with jQuery for years so my knowledge of it is extremely limited. I wrote the following form and received a comment elsewhere saying:


This is some of the worst JavaScript I've seen. Your selectors are
terrible ($('div#foo') instead of $('#foo')), you copy-paste code all
over the place, you return false instead of preventing the default
action alone, you manually concatenate your data string instead of
using the JSON library, you compare things with "==" instead of
"===".... This is just awful style and being a novice.

Most of the notes included in that comment doesn't make much sense to me, I haven't had my mind inside the insanity of Javascript for years now.

My question is, how do I upgrade my code to actually use good practice and up-to-date technique?

HTML:

  
    Name
    
    This field is required.

    Return Email
    
    This field is required.

    Return Phone
    
    This field is required.

      
    
  

Form validation:

$(function() {
    $('.error').hide();
    $(".button").click(function() {
      // validate and process form here

      $('.error').hide();
      var name = $("input#name").val();
        if (name == "") {
        $("label#name_error").show();
        $("input#name").focus();
        return false;
      }
        var email = $("input#email").val();
        if (email == "") {
        $("label#email_error").show();
        $("input#email").focus();
        return false;
      }
        var phone = $("input#phone").val();
        if (phone == "") {
        $("label#phone_error").show();
        $("input#phone").focus();
        return false;
      }

    });
  });

Process form:

```
var dataString = 'name='+ name + '&email=' + email + '&phone=' + phone;
//alert (dataString);return false;
$.ajax({
type: "POST",
url: "bin/process.php",
data: dataString,
success: function() {
$('#contact_form').html("");
$('#message').html("Contact Form Submitted!

In a sense, your code has already been reviewed, though I disagree with the excessively negative assessment. It is far from being awful and some of the worst JavaScript code ever.

-
you manually concatenate your data string instead of using the JSON library

As the documentation for $.ajax() says:


The data option can contain either a query string of the form key1=value1&key2=value2, or an object of the form {key1: 'value1', key2: 'value2'}. If the latter form is used, the data is converted into a query string using jQuery.param() before it is sent.

To create the query string from the form, you could take advantage of the .serialize() function.

So, that's what you should use instead. Concatenating your own data is wrong because you are failing to escape special characters. If any of the fields contains a =, &, or % character, the application will break — possibly in a way that introduces a security problem.

-
Your selectors are terrible ($('div#foo') instead of $('#foo'))

From the jQuery Learning Center:


Beginning your selector with an ID is always best. … ID-only selections are handled using document.getElementById(), which is extremely fast because it is native to the browser.

So do that.

-
you compare things with == instead of ===

The == operator does type conversion. In this case, all of your comparisons are between strings anyway. Using === would be better practice, but use of == in this code is not that harmful.

-
you return false instead of preventing the default action alone

Your version works, but event.preventDefault() is clearer.

-
copy-paste code all over the place

"All over the place" is an exaggeration. However, it would be better to extract repeated code into a function.

$('.button').click(function(event) {
    function checkRequiredFieldFilled(fieldSelector, errorSelector) {
        if ('' === $(fieldSelector).val()) {
            $(errorSelector).show();
            $(fieldSelector).focus();
            return false;
        }
        return true;
    }

    $('.error').hide();
    var ok = checkRequiredFieldFilled('#phone', '#phone_error') &
             checkRequiredFieldFilled('#email', '#email_error') &
             checkRequiredFieldFilled('#name',  '#name_error');
    if (!ok) event.preventDefault();
}

Note that this version validates all of the fields at once and focuses on the first field with a problem, to provide a better user experience.

StackExchange Code Review Q#74903, answer score: 13