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

Deleting a section of a website using Ajax

Submitted by: @import:stackexchange-codereview··
0
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.

$('.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 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 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 );
        });
    };
});

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 go
if(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.