Return to Answer

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.

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.

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 automatically create the query string of the form you could use 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.

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.

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.

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.

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 automatically create the query string of the form you could use 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.