Deleting a section of a website using Ajax

Question

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.

$('.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?

Answer 1

This is very weird. If I read the code without this line, I wouldn't have a clue what 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: <span data-id="example" />
var catid = this.id; // Just use id: <span id="example" />

---

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 go

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

• 099: hmm okay, thanks for the tips! A good while ago I got told that it was better to do something like var that = $(this) rather than reusing $(this) over and over.
• Martijn: I've updated my answer accordingly :)
• 099: nice! thanks for the response, I'll look into implementing this.
• phyrfox: By the way, $(elem).data('id') is actually equal to $(elem).attr('data-id'), while elem.id is the same as $(elem).attr('id'). You'll want to clear that up.
• Musa: @phyrfox $(elem).data('id') and $(elem).attr('data-id') can be changed independently of each other so saying they are equal is incorrect. see jsfiddle.net/s6tt7d3g