Simple jQuery Refactoring

Trevor Davis, Former Front-End Development Technical Director

Article Category: #Code

Posted on

Since we have a front-end development (FED) team at Viget now, we decided that we should have a weekly team meeting. We decided that we didn't want it to be just a typical meeting, so we have "FED Sacrifices". The way it works is that one person puts up a piece of code, gives a little background on what it does, and then everyone just tears it apart to figure out how to improve it. It has been fantastic, and now we argue over milliseconds of speed improvements. I am a much better coder because of it.

I've really only been using jQuery heavily for 2 or 3 years, but after about a year at Viget and a few months of FED Sacrifices, I have drastically changed how I use it. Before using jQuery, I did a fair amount of DOM scripting, but after seeing how much jQuery could simplify things, I made the switch. So I read through tutorials and copied code; that was how I learned how to use it. The code that I learned to write looked a lot like this:

Note: Line wraps added for readability.

$(document).ready(function() {
  $('#nav > li > a').click(function() {
    if($(this).parent().hasClass('current')) {
      $(this).parents('#nav').find('.current')
      .removeClass('current');
      $(this).parents('#nav').find('ul:visible').
      slideUp(250);
    } else {
      $(this).parents('#nav').find('.current').
      removeClass('current');
      $(this).parents('#nav').find('ul:visible').
      slideUp(250);
      $(this).parent().addClass('current');
      $(this).siblings('ul').slideDown(250);
    }
    return false;
  });
});

There are a few things wrong with this, so let's step through them one by one and see how we can improve it.

I've made a simple demo so that you can see the result of this code, but the functionality isn't really what this article is about.

Return False

When you are hijacking a link with JavaScript and you want to prevent the default action of the link (following the href), the first way you learned to do that was using return false. That works great, unless you end up having multiple event handlers attached to something. So maybe you have one event handler that toggles the menu state, and then another one that uses Google Analytics to track the event of the user clicking on that link. We can improve this code by using event.preventDefault().

First, we need to look at the click() event and see that inside of the anonymous function, we can also pass in the event object. The most common practice is probably to use e or event. Now with that event object, we can call the preventDefault() function.

$(document).ready(function() {
  $('#nav > li > a').click(function(e) {
    e.preventDefault();
    if($(this).parent().hasClass('current')) {
      $(this).parents('#nav').find('.current')
      .removeClass('current');
      $(this).parents('#nav').find('ul:visible')
      .slideUp(250);
    } else {
      $(this).parents('#nav').find('.current')
      .removeClass('current');
      $(this).parents('#nav').find('ul:visible')
      .slideUp(250);
      $(this).parent().addClass('current');
      $(this).siblings('ul').slideDown(250);
    }
  });
});

"Caching" jQuery Objects

One nice and easy way to speed up your jQuery is to cache jQuery objects in a variable. If we look at the code above, you can see that we are getting the jQuery object of this, a bunch of times. A better way to do this would be to store the jQuery object in a variable so that we only have to get the object once. A common practice when storing a jQuery object inside of a variable is to prefix the variable with a $ sign. This is an easy way to just look at the variable and see if it is a jQuery object vs another variable type.

$(document).ready(function() {
  $('#nav > li > a').click(function(e) {
    e.preventDefault();
    
    var $this = $(this);
    
    if($this.parent().hasClass('current')) {
      $this.parents('#nav').find('.current')
      .removeClass('current');
      $this.parents('#nav').find('ul:visible')
      .slideUp(250);
    } else {
      $this.parents('#nav').find('.current')
      .removeClass('current');
      $this.parents('#nav').find('ul:visible')
      .slideUp(250);
      $this.parent().addClass('current');
      $this.siblings('ul').slideDown(250);
    }
  });
});

We can take it one step further, and also cache some of the other jQuery objects that we are accessing more than once.

$(document).ready(function() {
  $('#nav > li > a').click(function(e) {
    e.preventDefault();
    
    var $this = $(this),
        $nav = $this.parents('#nav'),
        $parent = $this.parent();
    
    if($parent.hasClass('current')) {
      $nav.find('.current').removeClass('current');
      $nav.find('ul:visible').slideUp(250);
    } else {
      $nav.find('.current').removeClass('current');
      $nav.find('ul:visible').slideUp(250);
      $parent.addClass('current');
      $this.siblings('ul').slideDown(250);
    }
  });
});

Don't Use Shorthand Event Handlers

If you look at the jQuery source for the click() event, it's really just calling bind(). So we can save ourselves one function call by using bind() instead.

$(document).ready(function() {
  $('#nav > li > a').bind('click', function(e) {
    e.preventDefault();
    
    var $this = $(this),
        $nav = $this.parents('#nav'),
        $parent = $this.parent();
    
    if($parent.hasClass('current')) {
      $nav.find('.current').removeClass('current');
      $nav.find('ul:visible').slideUp(250);
    } else {
      $nav.find('.current').removeClass('current');
      $nav.find('ul:visible').slideUp(250);
      $parent.addClass('current');
      $this.siblings('ul').slideDown(250);
    }
  });
});

Pre-Processing Functions

We can pull some of this code outside of the event binding so that the function can be pre-processed before the document is ready.

function handleNav($el) {
  var $nav = $el.parents('#nav'),
      $parent = $el.parent();
  
  if($parent.hasClass('current')) {
    $nav.find('.current').removeClass('current');
    $nav.find('ul:visible').slideUp(250);
  } else {
    $nav.find('.current').removeClass('current');
    $nav.find('ul:visible').slideUp(250);
    $parent.addClass('current');
    $el.siblings('ul').slideDown(250);
  }
}
 
$(document).ready(function() {
  $('#nav > li > a').bind('click', function(e) {
    e.preventDefault();
    
    handleNav($(this));
  });
});

Namespace That Bro

We don't want to clutter up the global namespace with our function, so we should create an object to store all of our functions.

var VIGET = VIGET || {};
 
VIGET.handleNav = function($el) {
  var $nav = $el.parents('#nav'),
      $parent = $el.parent();
  
  if($parent.hasClass('current')) {
    $nav.find('.current').removeClass('current');
    $nav.find('ul:visible').slideUp(250);
  } else {
    $nav.find('.current').removeClass('current');
    $nav.find('ul:visible').slideUp(250);
    $parent.addClass('current');
    $el.siblings('ul').slideDown(250);
  }
};
 
$(document).ready(function() {
  $('#nav > li > a').bind('click', function(e) {
    e.preventDefault();
    
    VIGET.handleNav($(this));
  });
});

Don't Repeat Yourself

If you look at the first two lines of the if condition and the first two lines of the else condition, they are the exact same thing. By following the DRY principle, we should pull that code out into a separate function.

var VIGET = VIGET || {};
 
VIGET.handleNav = function($el) {
  var $nav = $el.parents('#nav'),
      $parent = $el.parent();
 
  VIGET.hideSubNav($nav);
  if(!$parent.hasClass('current')) {
    $parent.addClass('current');
    $el.siblings('ul').slideDown(250);
  }
};
 
VIGET.hideSubNav = function($nav) {
  $nav.find('.current').removeClass('current');
  $nav.find('ul:visible').slideUp(250);
};
 
$(document).ready(function() {
  $('#nav > li > a').bind('click', function(e) {
    e.preventDefault();
    
    VIGET.handleNav($(this));
  });
});

One Final Step

We can actually pull the e.preventDefault() call into the handleNav() function and avoid using the anonymous function in the $(document).ready.

var VIGET = VIGET || {};
 
VIGET.handleNav = function(e) {
  e.preventDefault();
  
  var $el = $(e.target),
      $nav = $el.parents('#nav'),
      $parent = $el.parent();
 
  VIGET.hideSubNav($nav);
  if(!$parent.hasClass('current')) {
    $parent.addClass('current');
    $el.siblings('ul').slideDown(250);
  }
};
 
VIGET.hideSubNav = function($nav) {
  $nav.find('.current').removeClass('current');
  $nav.find('ul:visible').slideUp(250);
};
 
$(document).ready(function() {
  $('#nav > li > a').bind('click', VIGET.handleNav);
});

Conclusion

This code accomplishes exactly the same thing as the first iteration but looks drastically different. There are some slight speed improvements, and we now have clearly defined functions that accomplish separate reusable tasks. By going back and refactoring your code as you go, you will end up with something much better than you started with.

Related Articles