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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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