Refactoring Code

Question

Let's say I have the following code:

    $(function () {
    $(".buy-it-now.ribbon").click(function () {
        $(".bid-to-beat.ribbon.active").removeClass("active");
        $(".bid-to-beat.ribbon").addClass("inactive");
        $(".buy-it-now.ribbon.inactive").removeClass("inactive");
        $(".buy-it-now.ribbon").addClass("active");
        $(".bid-now").hide();
        $(".buy-now").show();
        $(".add-to-cart").hide();
    })
    $(".bid-to-beat.ribbon").click(function () {
        $(".buy-it-now.ribbon.active").removeClass("active");
        $(".buy-it-now.ribbon").addClass("inactive");
        $(".bid-to-beat.ribbon").removeClass("inactive");
        $(".bid-to-beat.ribbon").addClass("active");
        $(".buy-now").hide();
        $(".bid-now").show();
        $(".add-to-cart").show();
    });
});

It is a simple function that allows for multiple UI related things to happen on the front-end of a site I am working on. I am fairly (very) new to jQuery and JavaScript in general and am learning about refactoring and making my code more condensed now. The way I currently write code is sort of line per thought I have. So my question is how would an experienced developer write this same code? Or rather, how could I refactor this code?

Ricardo Lohmann · Accepted Answer · 2012-09-12 20:15:42Z

1

Try the following:

$(function () {
    var $handlers = $('.buy-it-now.ribbon, .bid-to-beat.ribbon');

    $handlers.click(function() {
        $handlers.toggleClass("active inactive");

        var $elements = $(".bid-now, .add-to-cart"),
            $buyElement = $(".buy-now");

        if($(this).is('.buy-it-now.ribbon')) {
            $elements.hide();
            $buyElement.show();
        } else {
            $elements.show();
            $buyElement.hide();
        }
    });
});

answered Sep 12, 2012 at 20:15

Ricardo Lohmann

26.4k7 gold badges86 silver badges84 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Dain Over a year ago

Wow, that was amazing. This totally blew my mind. Thank you for the insight.

Sushanth -- Over a year ago

@RicardoLohmann , this is cool.. Never thought abt this approach !!

undefined Over a year ago

This looks to me like clicking the same .ribbon elements twice in a row would cause the active/inactive classes to get out of sync with the show/hidden elements. Also, @Ricardo, are you sure you need both .active and .inactive? Whatever is not active is inactive.

user400654 · Accepted Answer · 2012-09-12 19:53:00Z

0

This question would be better suited for codereview, but yes it can be condensed a little using method chaining.

$(function () {
    $(".buy-it-now.ribbon").click(function () {
        $(".bid-to-beat.ribbon").removeClass("active").addClass("inactive");
        $(".buy-it-now.ribbon").removeClass("inactive").addClass("active");
        $(".bid-now").hide();
        $(".buy-now").show();
        $(".add-to-cart").hide();
    })
    $(".bid-to-beat.ribbon").click(function () {
        $(".buy-it-now.ribbon").removeClass("active").addClass("inactive");
        $(".bid-to-beat.ribbon").removeClass("inactive").addClass("active");
        $(".buy-now").hide();
        $(".bid-now").show();
        $(".add-to-cart").show();
    });
});

You could condense it further by pre selecting the elements and caching them in variables before the click events as long as no elements are added or removed during the life of the page.

answered Sep 12, 2012 at 19:53

user400654

95.1k16 gold badges168 silver badges188 bronze badges

3 Comments

gen_Eric Over a year ago

You can combine it even more using toggleClass. $(".buy-it-now.ribbon").toggleClass("active inactive");. You can combine it even more, by combining selectors: $(".bid-now,.add-to-cart").show().

user400654 Over a year ago

i thought about toggle class, but wouldn't that not work as expected? the active one would become inactive, and all the inactive ones will become active, rather than them all becoming inactive. I do agree with combining selectors though, unless you run into performance problems in IE6/7 (don't know if that was ever fixed)

gen_Eric Over a year ago

.toggleClass("active inactive"); should work ok, as long as you don't ever have both (or neither) of those classes set at the same time.

Sushanth -- · Accepted Answer · 2012-09-12 21:01:24Z

0

As your code it is you can combine some of the selectors into a single line. And also because your elements looks to be static you can cache them into a variable and use them later as it reduces the number of times a element is looked up in the DOM reducing the accessing time..

Also you can limit the scope of these variables or selectors by encasing them in an object or a closure..

Maybe something in these lines..

       $(function () {
   cart.init();
});

var cart = {
    elems : {
        $buyRibbon : null,
        $bidRibbon : null,
        $bidNow: null,
        $buyNow: null,
        $addToCart: null
    },
    events : {
    },
    init : function() {
        this.elems.$buyRibbon = $(".buy-it-now.ribbon");
        this.elems.$bidRibbon = $(".bid-to-beat.ribbon");
        this.elems.$bidNow = $(".bid-now") ;
        this.elems.$buyNow = $(".buy-now") ;
        this.elems.$addToCart = $(".add-to-cart") ;
        this.events.buyClick();
        this.events.bidClick();
    }
};

cart.events.buyClick = function() {
    cart.elems.$buyRibbon.on('click', function(){
        cart.elems.$bidRibbon.removeClass('active').addClass('inactive');
        cart.elems.$buyRibbon.removeClass('inactive').addClass('active');
        cart.elems.$bidNow.hide();
        cart.elems.$buyNow.show();
        cart.elems.$addToCart.hide();
    });
} 

cart.events.bidClick = function() {
    cart.elems.$bidRibbon.on('click', function(){
        cart.elems.$buyRibbon.removeClass('active').addClass('inactive');
        cart.elems.$bidRibbon.removeClass('inactive').addClass('active');
        cart.elems.$bidNow.show();
        cart.elems.$buyNow.hide();
        cart.elems.$addToCart.show();
    });
}

So basically in here your whole cart is a object ..And the cart has different properties which are related to this.. You follow the principles of object oriented programming here.. Using closures I heard gives you better design limiting the scope of your code..

edited Sep 12, 2012 at 21:01

answered Sep 12, 2012 at 20:13

Sushanth --

55.8k9 gold badges70 silver badges109 bronze badges

10 Comments

Dain Over a year ago

Wow. This was incredibly impressive. I really appreciate you taking the time to show me this. I think I will use something similar to this, as it allows me a lot of reusability in the future.

Dain Over a year ago

Hmm. I'd like to use this code, but it doesn't work. :/ Not sure how to fix it, not advanced enough.

Sushanth -- Over a year ago

Do you see any errors in the console section of the browser??

Dain Over a year ago

Let me check. Attempting to debug now :)

Dain Over a year ago

Uncaught ReferenceError: $buyRibbon is not defined

|

undefined · Accepted Answer · 2012-09-12 21:15:43Z

Might I suggest something like this:

$(function () {
    var buyNowButton = $('buy-it-now.ribbon'),
        bidToBeatButton = $('.bid-to-beat.ribbon'),
        buyNowEls = $('.buy-now'),
        bidToBeatEls = $('.bid-now,.add-to-cart');

    var toggleButtons = function(showBuyNow){
        buyNowButton.toggleClass('active', showBuyNow);
        bidToBeatButton.toggleClass('active', !showBuyNow);
        buyNowEls.toggle(showBuyNow);
        bidToBeatEls.toggle(!showBuyNow);
    }

    buyNowButton.click(function(){ toggleButtons(true) });
    bidToBeatButton.click(function(){ toggleButtons(false) });
});

You could save a some lines by removing the selectors at the start and just do the selection in place, if the saved space would be more important than the minor performance hit. Then it would look like this:

$(function () {
    var toggleButtons = function(showBuyNow){
        $('buy-it-now.ribbon').toggleClass('active', showBuyNow);
        $('.bid-to-beat.ribbon').toggleClass('active', !showBuyNow);
        $('.buy-now').toggle(showBuyNow);
        $('.bid-now,.add-to-cart').toggle(!showBuyNow);
    }

    $('buy-it-now.ribbon').click(function(){ toggleButtons(true) });
    $('.bid-to-beat.ribbon').click(function(){ toggleButtons(false) });
});

The first version selects the elements once and holds them in memory; the second selects them each time the button is clicked. Both solve the problem I believe would occur with the selected answer where clicking the same button twice would cause the .active and .inactive classes to get out of sync with the shown/hidden elements.

Collectives™ on Stack Overflow

Refactoring Code

4 Answers 4

3 Comments

3 Comments

10 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

3 Comments

3 Comments

10 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related