Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

});
share|improve this question
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
add comment

2 Answers 2

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.
share|improve this answer
    
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
    
(I meant "answers".) –  Quentin Pradet Feb 19 '13 at 16:01
    
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
add comment

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

share|improve this answer
    
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
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.