3
\$\begingroup\$

I have this code that basically reads the DOM and sends the values to the server. I am looking for any possible flaws that I may have in this JavaScript code and any advice to make it better and bug-free!

$(document).ready(function () {
    $('.editButton').click(function () {
        var postData = {};
        var cData = {};
        cData.Balance = $(this).parent().children('.balance').text().replace("$", "");
        cData.desiredStatus = $(this).parent().children('.status').text() == "Disbled" ? "enable" : "disable";
        $('#currentBalance').html($(this).parent().children('.balance').text());
        $('#currentStatus').html($(this).parent().children('.status').text());
        $('#desiredStatus').html(cData);

        //set post view
        postData.code = $(this).parent().children('.code').text();
        postData.giftcardaccount_id = $(this).parent().children('.giftcardaccount_id').text();


        $("#dialog").dialog({
            title:"test box",
            modal:true,
            width:700,
            buttons:{
                'Confirm':function () {
                    postData.status = $('#currentStatus').html();
                    var balanceInqury = parseFloat($('#desiredBlance').val());
                    postData.balance = (balanceInqury == 'NaN') ? cData.Balance : balanceInqury;
                    $.ajax({
                        url:'/url/to/postto',
                        type:'POST',
                        data:{
                            postData:postData
                        },
                        success:function (data) {
                            console.log(data);
                            $("#dialog").dialog("close");

                        },
                        error:function () {
                        }
                    });
                },
                Cancel:function () {
                    $(this).dialog("close");
                }
            }
        });//dialog
    });


    $('#changeStatus').click(function () {
        var c = $('#currentStatus').html() == 'Disabled' ? 'Enabled' : 'Disabled';
        var d = $('#desiredStatus').html() == 'disable' ? 'enable' : 'disable';
        $('#currentStatus').html(c);
        $('#desiredStatus').html(d);

    });

});
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
  • You're creating a lot of jQuery objects in there. It would be much more efficient to just create them once. For example:

    var postData = {},
        cData = {},
        $this = $(this),
        $parent = $this.parent(),
        $balance = $parent.children(".balance");
    

As a general rule, I try to avoid calling any method on the same object more than once. For any methods you need to call repeatedly, just call it once and store the result.

  • The following line seems to be incorrect. You're passing an object to the .html() method (which it doesn't accept):

    $('#desiredStatus').html(cData);
    
    //Did you mean to do this instead?
    $('#desiredStatus').html(cData.desiredStatus);
    
  • You are creating a new modal dialog every time a .editButton element is clicked. You could move the creation of the dialog outside of the event handler and just call $("#dialog").dialog("open") when you want it to appear.

  • The comparison balanceInqury == 'NaN' will never be true. JavaScript is funny like that, NaN !== NaN. There is a built-in isNaN function you can use though:

    postData.balance = isNaN(balanceInqury) ? cData.Balance : balanceInqury;
    
  • For shorter code you could replace your call to $.ajax with a call to the shorthand $.post method. If you'd rather not do that, you can at least remove the error property from the options object, since it doesn't do anything in your case:

    $.post('/url/to/postto', { postData: postData}, function (data) {
        //You make another unnecessary jQuery object in here. Cache the #dialog object!
    });
    
  • Finally, you could change the code in the #changeStatus click event handler by passing a function to the .html method, which in my opinion is a bit neater:

    $('#currentStatus').html(function () {
        return $(this).html() === "Disabled" ? "Enabled" : "Disabled"
    });
    $('#desiredStatus').html(function () {
        return $(this).html() === "disable" ? "enable" : "disable";
    });
    
\$\endgroup\$
2
  • \$\begingroup\$ I am not sure what do you mean by creating the dialog box before click...how would I get the values in which are not there until the button is clicked. thanks \$\endgroup\$
    – Autolycus
    Commented Oct 3, 2012 at 16:02
  • \$\begingroup\$ @Autolycus - You could use the .option() method to set the buttons in the event handler. Not sure if it would be any quicker or not... you can run benchmarks at jsperf.com. \$\endgroup\$ Commented Oct 3, 2012 at 16:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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