patternjavascriptMinor
Deleting a section of a website using Ajax
Viewed 0 times
websitedeletingajaxsectionusing
Problem
I am currently writing a decent amount of ajax to help me delete a section of a website.
It's already becoming a pretty big piece of code, I wondered if it were possible to compact a section of my code.
Is it possible to compact this code, or is this as small as it can get?
It's already becoming a pretty big piece of code, I wondered if it were possible to compact a section of my code.
$('.trg-finished').on('click', function() {
if(confirm('Are you sure?')) {
var that = $(this);
var catid = that.data("id");
$.ajax({
type: "POST",
url: "./close-cat.php",
data: { catID: catid }
})
.done(function(data) {
if(data == 1) {
that
.css("background-color", "#27AE60")
.css("color", "#FFF")
.html('Success');
} else {
that
.css("background-color", "#C0392B")
.css("color", "#FFF")
.html('Failure');
};
window.setTimeout(function() {
that
.css("background-color", "")
.css("color", "")
.html('Finished');
}, 5000);
})
.fail(function(data) {
that
.css("background-color", "#C0392B")
.css("color", "#FFF")
.html('Failure');
window.setTimeout(function() {
that
.css("background-color", "")
.css("color", "")
.html('Finished');
}, 5000);
});
};
});Is it possible to compact this code, or is this as small as it can get?
Solution
This is very weird. If I read the code without this line, I wouldn't have a clue what
Saving the object is a good thing, saving multiple DOM look-ups, just be obvious about the name.
Instead of getting the
You chain your css methods, you can use them more effectively by using the multiple input variant:
Even better, do this via classes. You should not style with javascript, apart from tiny exceptions.
This way you can style via css, as you should. This also allows simple css animation.
Instead of calling the timeout, use a function. DRY (=don't repeat yourself) programming is often more maintainable (IMO):
All suggestions combined:
that might be:var that = $(this); // this is very weird
// I suggest the following name, (or thisElement, or something like that)
var $elem = $(this); // I use the $ in the name to indicate 'jQuery Object'Saving the object is a good thing, saving multiple DOM look-ups, just be obvious about the name.
Instead of getting the
data-id, you could use this to get the id of your element, which you can access it directly via JS, which is a lot faster:var catid = that.data("id"); // instead of this:
var catid = this.id; // Just use id: You chain your css methods, you can use them more effectively by using the multiple input variant:
that
.css({backgroundColor: "#27AE60", color: "#FFF"}) // multiple in one goEven better, do this via classes. You should not style with javascript, apart from tiny exceptions.
if(data == 1) {
that.addClass("HighlightGood").html('Success');
} else {
that.addClass("HighlightBad").html('Failure');
};
window.setTimeout(function() {
that.removeClass("HighlightGood HighlightBad").html('Finished');
}, 5000);This way you can style via css, as you should. This also allows simple css animation.
Instead of calling the timeout, use a function. DRY (=don't repeat yourself) programming is often more maintainable (IMO):
function setAsFinished( $elem, timeout ){
timeoutLength = timeout || 5000; // Take the given input (if set), else default value 5sec
window.setTimeout(function() {
$elem.removeClass("HighlightGood HighlightBad").html('Finished');
}, timeoutLength);
}All suggestions combined:
function setAsFinished( $elem, timeout ){
timeoutLength = timeout || 5000; // Take the given input (if set), else default value 5sec
window.setTimeout(function() {
$elem.removeClass("HighlightGood HighlightBad").html('Finished');
}, timeoutLength);
}
$('.trg-finished').on('click', function() {
if( confirm('Are you sure?') ){
var $thisElem = $(this);
var catid = this.id;
$.ajax({
type: "POST",
url: "./close-cat.php",
data: { catID: catid }
})
.done(function(data) {
if(data == 1) {
$thisElem.addClass("HighlightGood").html('Success');
} else {
$thisElem.addClass("HighlightBad").html('Failure').html('Failure');
};
setAsFinished( $thisElem );
})
.fail(function(data) {
$thisElem.addClass("HighlightBad").html('Failure');
setAsFinished( $thisElem );
});
};
});Code Snippets
var that = $(this); // this is very weird
// I suggest the following name, (or thisElement, or something like that)
var $elem = $(this); // I use the $ in the name to indicate 'jQuery Object'var catid = that.data("id"); // instead of this: <span data-id="example" />
var catid = this.id; // Just use id: <span id="example" />that
.css({backgroundColor: "#27AE60", color: "#FFF"}) // multiple in one goif(data == 1) {
that.addClass("HighlightGood").html('Success');
} else {
that.addClass("HighlightBad").html('Failure');
};
window.setTimeout(function() {
that.removeClass("HighlightGood HighlightBad").html('Finished');
}, 5000);function setAsFinished( $elem, timeout ){
timeoutLength = timeout || 5000; // Take the given input (if set), else default value 5sec
window.setTimeout(function() {
$elem.removeClass("HighlightGood HighlightBad").html('Finished');
}, timeoutLength);
}Context
StackExchange Code Review Q#98230, answer score: 7
Revisions (0)
No revisions yet.