# jQuery form data code refinement

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); } });  - Not sure where the bug is, but it looks much better already. I would just call moveDiv() instead of $.change(moveDiv); –  konijn Feb 19 '13 at 16:56
Thanks so much, tomdemuyt, that changed everything... Have a great day tomorrow. J –  James Feb 19 '13 at 23:59

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.
-
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 –  James Feb 19 '13 at 15:08
@James glad you like it! Consider upvoting questions that helped you now that you have 15 reputation. :) –  Quentin Pradet Feb 19 '13 at 15:20
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? –  James Feb 19 '13 at 16:07

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

-
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. –  James Feb 19 '13 at 16:04