Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm using two category filters from this demo. I give the filters two different sets of classes so they won't affect each other. Since I'm very new to jQuery, all I can come up with is repeating the same code with a different set of classes. I wonder if there's a shorter way to write this:

$(function(){

    /* filter1 */
    $(".category-menu ul li").click(function(){
        var CategoryID = $(this).data('category');
        $('.category-menu ul li').removeClass('cat-active');
        $(this).addClass('cat-active');

        $('.prod-cnt').each(function(){
            if(($(this).hasClass(CategoryID)) == false){
               $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 

    });

    $('.category-menu select').change(function(){
        var CategoryID = $(this).find('option:selected').data('category');
        $('.prod-cnt').each(function(){
            if(($(this).hasClass(CategoryID)) == false){
               $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 
    });

    /* filter2 */
    $(".category-menu2 ul li").click(function(){
        var CategoryID = $(this).data('category');
        $('.category-menu2 ul li').removeClass('cat-active2');
        $(this).addClass('cat-active2');

        $('.prod-cnt2').each(function(){
            if(($(this).hasClass(CategoryID)) == false){
               $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 

    });

    $('.category-menu2 select').change(function(){
        var CategoryID = $(this).find('option:selected').data('category');
        $('.prod-cnt2').each(function(){
            if(($(this).hasClass(CategoryID)) == false){
               $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 
    });

});

Please check out this demo.

share|improve this question

1 Answer 1

up vote 8 down vote accepted

When you see that you're basically copy-pasting the code and changing a few values, you can usually encapsulate all the common stuff inside a function. Here:

var registerStuff = function(category, active, prod, select) {
    $(category).click(function(){
        var CategoryID = $(this).data('category');
        $(category).removeClass(active);
        $(this).addClass(active);

        $(prod).each(function(){
            if(($(this).hasClass(CategoryID)) == false){
               $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 
    });

    $(select).change(function(){
        var CategoryID = $(this).find('option:selected').data('category');
        $(prod).each(function(){
            if(($(this).hasClass(CategoryID)) == false){
                $(this).css({'display':'none'});
            };
        });
        $('.'+CategoryID).fadeIn(); 
    });
};

registerStuff(".category-menu  ul li", 'cat-active',  '.prod-cnt',  '.category-menu  select');
registerStuff(".category-menu2 ul li", 'cat-active2', '.prod-cnt2', '.category-menu2 select');

We can now clean up the inside of that function further. For example, some things could be cached, and the second $(category) is uneccessary. The == false is a code smell, rather use a negation. The jQuery hide() method is better than spelling out the css(). Blending out the prods of the wrong category is common code, and can be factored out as well.

var registerStuff = function(category, active, prod, select) {
    var displayOnlyWantedCategory = function($prods, categoryId) {
        $prods.each(function() {
            var $this = $(this);
            if (! $this.hasClass(categoryId)) {
                $this.hide();
            }
        });
        $('.' + categoryId).fadeIn(); 
    };

    var $category = $(category);
    $category.click(function() {
        var $this = $(this);
        $category.removeClass(active);
        $this.addClass(active);

        displayOnlyWantedCategory($(prod), $this.data('category'));
    });

    $(select).change(function() {
        displayOnlyWantedCategory($(prod), $(this).find('option:selected').data('category'));
    });
};

The next thing to consider is your usage of classes. Your category-menu and category-menu2 are visually and functionally identical, they're only two instances of the same idea. What happens when you want to have fifteen category-menus on the same page? I wouldn't want to see the CSS for that:

 .category-menu,
 .category-menu2,
 .category-menu3,
   ...
 .category-menu14,
 .category-menu15 {
   ...
 }

Your solution does not scale. Instead, let's think about a product-collection element, which contains a few buttons to select a category, and a few items which can be displayed. We want to limit the effect of selecting a category to the current collection only.

The resulting HTML might look like:

<div class="product-collection">
  <ul>
    <li class="product-collection__selector
               product-collection__selector--active"
        data-product-collection__category="">All</li>
    <li class="product-collection__selector"
        data-product-collection__category="cat1">Category 1</li>
    <li class="product-collection__selector"
        data-product-collection__category="cat2">Category 2</li>
  </ul>
  <ul>
    <li class="product-collection__item"
        data-product-collection__category="cat1">Item 1 [cat 1]</li>
    <li class="product-collection__item"
        data-product-collection__category="cat2">Item 2 [cat 2]</li>
    <li class="product-collection__item"
        data-product-collection__category="cat1">Item 3 [cat 1]</li>
  </ul>
</div>

The jQuery initialization must take care to install the handlers only in the elements of the current collection:

$(function() {
    $('.product-collection').each(function() {
        var $collection = $(this);
        var $selectors = $collection.find('.product-collection__selector');
        var $items     = $collection.find('.product-collection__item');

        $selectors.click(function() {
            var $selector = $(this);
            var cat = $selector.data('product-collection__category');

            $selectors.removeClass('product-collection__selector--active');
            $selector.addClass('product-collection__selector--active');

            if (cat) {
                $items.each(function() {
                    var $item = $(this);
                    if ($item.data('product-collection__category') == cat)
                        $item.fadeIn();
                    else
                        $item.hide();
                });
            }
            else {
                $items.fadeIn();
            }
        });
    });
});

See it in action here: http://jsfiddle.net/Qv6QE/3/

My class names may seem excessively verbose, but they avoid namespace conflicts by following the Block-Element-Modifier naming scheme.

share|improve this answer
    
excellent answer! Easy to understand for a novice like me! I'll need some time to digest and make a few tries. I'll give you a vote up as soon as I have 15 reputation. Thank you! –  user35295 Feb 13 at 12:59
3  
Quite a nice review, only last thing I'd change is I would probably write $item.css({ display: 'none' }); as $item.hide(). Good stuff! Also I would probably write you product-collection__selector--active as two classes product-collection and active, I'm not a fan of the BEM scheme for non enterprise size projects/plugins –  megawac Feb 13 at 15:07
    
@amon The only thing I miss about the old one is that the posts can be filtered by classes, which means that as long as the word is one of the classes, the posts can be filtered, which allows me to have different types of filters. But in your version, the data-product-collection__categories of the filters and posts have to be the exact match. Is there any way to change it back to "lazy match"? –  user35295 Feb 13 at 18:50
1  
@user35295 It's still quite straightforward to have multiple categories per item, you just have to spell out the necessary JS yourself. See jsfiddle.net/cYFLe for an example. Do not use classes when they are not appropriate. The category of an item is data, and it is in no way relevant to styling. –  amon Feb 13 at 21:14
    
@amon Thanks a lot. I've been trying to add a few select boxes as additional filters. But I'm struggling to make it change its background color on click like the old buttons and then back to its original color when I click on any other button or select box. The effect is the same as in this example. Do you know what's wrong with this fiddle? I've tried creating another var for the select boxes but no joy... –  user35295 Feb 14 at 10:39

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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