5
\$\begingroup\$

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?

\$\endgroup\$
0

1 Answer 1

7
\$\begingroup\$

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 );
        });
    };
});
\$\endgroup\$
7
  • \$\begingroup\$ 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. \$\endgroup\$
    – 099
    Commented Jul 27, 2015 at 14:47
  • \$\begingroup\$ I've updated my answer accordingly :) \$\endgroup\$
    – Martijn
    Commented Jul 27, 2015 at 15:12
  • \$\begingroup\$ nice! thanks for the response, I'll look into implementing this. \$\endgroup\$
    – 099
    Commented Jul 27, 2015 at 15:41
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – phyrfox
    Commented Jul 27, 2015 at 18:56
  • \$\begingroup\$ @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 \$\endgroup\$
    – Musa
    Commented Jul 27, 2015 at 19:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.