jQuery AJAX form using bad practice

Question

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:

<div id="contact_form">
<form name="contact" action="">
  <fieldset>
    <label for="name" id="name_label">Name</label>
    <input type="text" name="name" id="name" size="30" value="" class="text-input" />
    <label class="error" for="name" id="name_error">This field is required.</label>

    <label for="email" id="email_label">Return Email</label>
    <input type="text" name="email" id="email" size="30" value="" class="text-input" />
    <label class="error" for="email" id="email_error">This field is required.</label>

    <label for="phone" id="phone_label">Return Phone</label>
    <input type="text" name="phone" id="phone" size="30" value="" class="text-input" />
    <label class="error" for="phone" id="phone_error">This field is required.</label>

      <br />
    <input type="submit" name="submit" class="button" id="submit_btn" value="Send" />
  </fieldset>
</form>
</div>

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("<div id='message'></div>");
      $('#message').html("<h2>Contact Form Submitted!</h2>")
      .append("<p>We will be in touch soon.</p>")
      .hide()
      .fadeIn(1500, function() {
        $('#message').append("<img id='checkmark' src='images/check.png' />");
      });
    }
  });
  return false;

Answer 1

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.

Answer 2

As far as am concern, your code is not too bad. But instead of manual concatenate of your data, you could user form.serialize(), or even create an object link so var mydata = {mail:mail,phone:phone};