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); }

});

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.

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