HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Read DOM and send values to server

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
valuesreadserverdomandsend

Problem

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

$('#chang

Solution

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

Code Snippets

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

//Did you mean to do this instead?
$('#desiredStatus').html(cData.desiredStatus);
postData.balance = isNaN(balanceInqury) ? cData.Balance : balanceInqury;
$.post('/url/to/postto', { postData: postData}, function (data) {
    //You make another unnecessary jQuery object in here. Cache the #dialog object!
});
$('#currentStatus').html(function () {
    return $(this).html() === "Disabled" ? "Enabled" : "Disabled"
});
$('#desiredStatus').html(function () {
    return $(this).html() === "disable" ? "enable" : "disable";
});

Context

StackExchange Code Review Q#16135, answer score: 2

Revisions (0)

No revisions yet.