jQuery form data

Question

Could someone please help me make this code as professional as it can be? It works fine, but I feel there's a better way of doing this. I would really appreciate some guidance so I learn to code better.

 $(function () {
 $('#subForm').submit(function (e) {
 e.preventDefault();
 $.getJSON(
 this.action + "?callback=?",
 $(this).serialize(),
 function (data) {
 if (data.Status === 400) {
 $('#slidemarginleft p').append("" + data.Message);
 $("#slidemarginleft p").append('<button class="css3button wee">OK&#44; I&#39;ll try again</button>');
 $(function() {
 var $marginLefty = $('.inner');
 $marginLefty.animate({
 marginLeft: parseInt($marginLefty.css('marginLeft'),10) == 0 ?
 $marginLefty.outerWidth() : 0
 });
 });
 //add click functionality for the button
 $('#slidemarginleft button.wee').click(function() {
 var $marginLefty = $('.inner');
 $marginLefty.animate({
 marginLeft: parseInt($marginLefty.css('marginLeft'),10) == 0 ?
 $marginLefty.outerWidth() : 0
 }); 
 setTimeout(function() {
 $('#slidemarginleft p').empty();}, 500);
 }); 
 } else { // 200
 $(function() {
 var $marginLefty = $('.inner');
 $marginLefty.animate({
 marginLeft: parseInt($marginLefty.css('marginLeft'),10) == 0 ?
 $marginLefty.outerWidth() :
 0
 });
 });
 $('#slidemarginleft p').append("Thank you. " + data.Message);
 }
 });
 });
 if ($.browser.webkit) { $input.css('marginTop', 1); }
 });

I have amended the code to this now (watching my indentation, adding a function, moving the default behaviour down but I think I have done it incorrectly as it doesn't work now):

$(function () {
 "use strict";
 $('#subForm').submit(function (e) {
 function moveDiv() {
 var $marginLefty = $('.inner');
 $marginLefty.animate({
 marginLeft: parseInt($marginLefty.css('marginLeft'), 10) === 0 ?
 $marginLefty.outerWidth() : 0
 });
 }
 $.getJSON(
 this.action + "?callback=?",
 $(this).serialize(),
 function (data) {
 if (data.Status === 400) {
 $('#slidemarginleft p').append(" " + data.Message);
 $("#slidemarginleft p").append('<button class="css3button wee">OK&#44; I&#39;ll try again</button>');
 $.change(moveDiv);
 //add click functionality for the button
 $('#slidemarginleft button.wee').click.change(moveDiv);
 setTimeout(function () {
 $('#slidemarginleft p').empty();
 }, 500);
 } else { // 200
 $.change(moveDiv);
 $('#slidemarginleft p').append("Thank you. " + data.Message);
 }
 });
 e.preventDefault();
 });
 //if ($.browser.webkit) { $.browser.input.css('marginTop', 1); }
});

Here is the final code that works like a dream:

$(function () {
 "use strict";
 $('#subForm').submit(function (e) {
 function moveDiv() {
 var $marginLefty = $('.inner');
 $marginLefty.animate({
 marginLeft: parseInt($marginLefty.css('marginLeft'), 10) === 0 ?
 $marginLefty.outerWidth() : 0
 });
 }
 $.getJSON(
 this.action + "?callback=?",
 $(this).serialize(),
 function (data) {
 if (data.Status === 400) {
 $('#slidemarginleft p').append(" " + data.Message);
 moveDiv();
 //add click functionality for the button
 $('#slidemarginleft button.wee').click(function () {
 moveDiv();
 setTimeout(function () {
 $('#slidemarginleft p').empty();
 }, 500);
 });
 } else { // 200
 moveDiv();
 $('#slidemarginleft p').append("Thank you. " + data.Message);
 }
 });
 e.preventDefault();
 });
 //if ($.browser.webkit) { $.browser.input.css('marginTop', 1); }
});

• konijn: Not sure where the bug is, but it looks much better already. I would just call moveDiv() instead of $.change(moveDiv)
• James: Thanks so much, tomdemuyt, that changed everything... Have a great day tomorrow. J

Answer 1

This is typical jQuery mess. If you write a lot of code like this every day, consider switching to frameworks like Backbone or Ember.js which will make your life easier. Concerning this code, here are my remarks:

• Care more about indentation! All those callbacks are hard enough to read as they are.
• Only put e.preventDefault(); at the end. If there's a bug in the JS code, the user will be able to use the fallback, that is submitting the form without JS.
• Don't build html elements with string concatenations, but write things like $('<button/>').addClass('css3button') and so on.

• Move this code into its own function:

  var $marginLefty = $('.inner');
  $marginLefty.animate({
   marginLeft: parseInt($marginLefty.css('marginLeft'),10) == 0 ?
   $marginLefty.outerWidth() : 0
  });
  

• More generally try to separate logic functions from display functions.

• James: Thank you so much for some guidance, tomdemuyt and Cygal...I really appreciate your time. I'm just learning (bodging) so this is infinately helpful in growing my knowledge. J
• Quentin Pradet: @James glad you like it! Consider upvoting questions that helped you now that you have 15 reputation. :)
• Quentin Pradet: (I meant "answers".)
• James: hehe, I know, I have upvoted now right? Is the indentation better now? I have also put the function in a named function now but it doesn't work, any thoughts? I'm guessing I'm calling it incorrectly?

Answer 2

In no particular order, these strike me as most worthy of solving:

• HTTP return codes, I would check for 200 and not-200, I would not assume that not-400 means 200 (when your web server dies, it will return 500's..)

• DRY Dont Repeat Yourself: the animation code seems copy pasted and could use a dedicated function

• Personally I would have the retry button be loaded with the html, with a 'hidden' css, less bytes, cleaner. Then you just have to unhide that button and voila..

• As Cygal said: fix your indentation

• James: Hey thanks Tomdemuyt I'm gonna look to integrate your thoughts once I get this amended function code working, thanks for your advice. I have fixed the indentation haven't I? I have used JSLint to help.