jQuery: refactoring lazy code

Question

I've written a function like so which toggles elements on and off:

  // category filter
  $('.category-filter').click(function() {
    var category = $(this).attr('data-category');
    $('.category-filter').each(function() {
      if ($(this).attr('data-category').indexOf(category) >= 0) {
        $(this).removeClass('label-default').addClass('label-success').fadeIn('fast');
        $(this).click(function() {
          $(this).removeClass('label-success').addClass('label-default').fadeIn('fast');
          $('.article_container').each(function() { $(this).fadeIn('fast') })
          $('.category-filter').each(function() { $(this).fadeIn('fast') })
        })
      }
    })
    $('.article_container').each(function() {
      if (!($(this).attr('data-categories').indexOf(category) >= 0)) {
        $(this).fadeOut('fast')
      }
    })
  });

It works perfectly but it seems messy to me. In fact, I've got a lot of functions in my app that look similar. Is there a style guide or something similar to help me refactor this code? How should it be refactored anyway?

I found this but I'm not sure where to start with mine.

EDIT

I made a jsfiddle to illustrate the issue. Essentially the buttons should eliminate "articles" not relevant to the button. It should work like a filter. Presently the fiddle isn't working correctly because I'm halfway through refactoring.

just name your functions to get them out of the flow mass and allow recycling, like naming function() { $(this).fadeIn('fast') } as fadeFast, and calling it from above. if you got rid of all the anons, the code would be quite legible and logical. — dandavis
– dandavis, Commented Apr 16, 2015 at 2:13
If you're refactoring anyway, might I suggest looking into AngularJS? — sg.cc
– sg.cc, Commented Apr 16, 2015 at 2:16
If you just want suggestions for how to improve this code, then you should probably go to codereview.stackexchange.com. — jfriend00
– jfriend00, Commented Apr 16, 2015 at 2:35

tandrewnichols · Accepted Answer · 2015-04-16 03:06:29Z

3

Hard to know what could be simplified without understanding what it does, but I would definitely recommend saving off jquery selectors as variables so you don't have to pay the DOM reading cost more than once. People often like to prefix such variables with $ to indicate that they are jquery objects, but this is really just preference. Also, never underestimate the importance of spacing.

var categoryFilter = $('.category-filter');
var articleContainer = $('.article-container');

categoryFilter.click(function() {
  var category = $(this).attr('data-category');

  categoryFilter.each(function() {
    if ($(this).attr('data-category').indexOf(category) >= 0) {
      $(this).removeClass('label-default').addClass('label-success').fadeIn('fast');

      $(this).click(function() {
        $(this).removeClass('label-success').addClass('label-default').fadeIn('fast');
        articleContainer.each(function() { $(this).fadeIn('fast') })
        categoryFilter.each(function() { $(this).fadeIn('fast') })
      })
    }
  });

  articleContainer.each(function() {
    if (!($(this).attr('data-categories').indexOf(category) >= 0)) {
      $(this).fadeOut('fast')
    }
  })
});

EDIT:

Having looked at this a bit more, I see a couple things you could simplify.

1) As @dandavis mentioned, you can create a named function for your fadeIn('fast') calls.

function fadeFast() {
  $(this).fadeIn('fast');
}

2) You could also create a named function called swapClass to handle your removeClass().addClass() cases.

function swapClass(context, rm, add) {
  return context.removeClass(rm).addClass(add);
}

Then invoke it like so:

swapClass($(this), 'label-default', 'label-success').fadeIn('fast');

But it really feels like that should actually be on the jquery instance (since you have to pass that in as the context), which you can do like this:

$.fn.swapClass = function(oldClass, newClass) {
  return this.removeClass(oldClass).addClass(newClass);
};

Then invoke it like this:

$(this).swapClass('label-default', 'label-success').fadeIn('fast');

So that gets you to something like:

var categoryFilter = $('.category-filter');
var articleContainer = $('.article-container');

$.fn.swapClass = function(oldClass, newClass) {
  return this.removeClass(oldClass).addClass(newClass);
};

function fadeFast() {
  $(this).fadeIn('fast');
}

categoryFilter.click(function() {
  var category = $(this).attr('data-category');

  categoryFilter.each(function() {
    var $this = $(this);
    if ($this.attr('data-category').indexOf(category) >= 0) {
      $this.swapClass('label-default', 'label-success').fadeIn('fast');

      $this.click(function() {
        $(this).swapClass('label-success', 'label-default').fadeIn('fast');
        articleContainer.each(fadeFast);
        categoryFilter.each(fadeFast);
      })
    }
  });

  articleContainer.each(function() {
    var $this = $(this);
    if (!($this.attr('data-categories').indexOf(category) >= 0)) {
      $this.fadeOut('fast')
    }
  })
});

edited Apr 16, 2015 at 3:06

answered Apr 16, 2015 at 2:23

tandrewnichols

3,4561 gold badge31 silver badges33 bronze badges

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

3 Comments

jfriend00 Over a year ago

This doesn't seem to address what the OP was asking for.

tandrewnichols Over a year ago

It depends what you mean by "refactor." I agree, generally. I think the OP is asking about ways to simplify the code, but it's hard to know for sure, without understanding more fully what it does. My point here was just to show that you can make some improvement without actually doing anything at all. For instance, I think it would be much easier to see where to begin real refactoring in my version than in the original, just because of the spacing. Furthermore, it was too long to fit in a comment.

dandavis Over a year ago

there are still a lot of repeats going on. don't be shy, call your functions by name. there's no reason you need the word "function" more than a couple times inside of the click event. ok, you technically don't need it at all, but let's not get carried away: abstracting the boilerplate will be beneficial enough.

jfriend00 · Accepted Answer · 2015-04-16 04:47:55Z

Here's a different way to approach things that creates a class name from the data-category attribute to make it easier to select like items with a simple selector:

$('.category-filter').each(function() {
    // move data-category attribute to a class name so it's easier to select
    $(this).addClass("cat_" + $(this).data("category"));
}).click(function(e){
    var categoryClass = ".cat_" + $(this).data('category');
    // now change all items with matching category
    $(categoryClass).each(function() {
        $(this).removeClass('label-default').addClass('label-success').fadeIn('fast');
          .click(function() {
            $(this).removeClass('label-success').addClass('label-default');
            $('.article_container').fadeIn('fast');
            $('.category-filter').fadeIn('fast');
          });        
    });
    $('.article_container').not(categoryClass).fadeOut('fast');
});

List of Changes:

Copy data-category attribute to a class name to make it easier to select all matching category elements.
Use chaining in more places to avoid repeating jQuery object creation.
Remove some redundant .fadeIn() calls.
Use the new class name to fadeOut the right articles

I think your code has an issue with installing duplicate click handlers each time a category-filter item is clicked on. I don't quite understand why you're doing it the way you are so I didn't change that part yet.

Why the downvote? I'm showing another way to approach the problem.

Collectives™ on Stack Overflow

jQuery: refactoring lazy code

2 Answers 2

3 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related