patternjavascriptMinor
Read DOM and send values to server
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
```
$(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:
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
-
You are creating a new modal dialog every time a
-
The comparison
-
For shorter code you could replace your call to
-
Finally, you could change the code in the
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.