jQuery code smells

Here’s a quick line-up of some smelly jQuery code! “Code smells” are pieces of code that do for your eyes what bad smells do for your nostrils, and usually result in erroneous or harder-to-maintain code. I have no doubt that at least half of you will think that I’m wrong about at least half of these.

Before you stop paying attention, I’d like to wish everyone a Happy New Year! I hope you find the new design more consistent and cleaner.

May the flaming commence…

Prefixxing all jQuery objects with “$”

It’s ugly, and my text editor doesn’t like it! But mainly, it’s ugly, and is only useful for beginners. Hungarian notation is a hotly debated technique — like Marmite (and Jade Goody?), you either love it or hate it!

var $body = $('body');
$body.click(function(){
    $this = $(this);
});

I won’t say much more; I appreciate that it’s a sensitive topic for some and is mostly about personal preference.

Concatenating a bunch of things together to make a selector

I see a lot of this! It looks dirty, and just screams, “I’m succumbing to the API because I don’t know what else to do!”

$('.' + className + '[' + attrLookup + '^=' + attrPrefix ']:not(.' + notClass + ')');

This is why I think jQuery needs some other way of filtering DOM elements (possibly something like my filter hack). For now, seperating out our selectors will have to suffice:

$('.' + blah.className)
    .filter('[' + attrLookup + '^=' + attrPrefix ']')
    .not('.' + notClassName);

To be truthful, I’m on the fence with this… Maybe I’m just against string concatenation!

Going over the top with chaining

Some developers have obviously been dared to never exit a chain, ever!

$('a').addClass('reg-link')
    .find('span')
        .addClass('inner')
    .end().end()
    .find('div')
        .mouseenter(mouseEnterHandler)
        .mouseleave(mouseLeaveHandler)
    .end()
    .explode();

Long chains are okay, as long as you’re not doing a massive amount of DOM traversing in them… they can become unwieldy beasts!

Not caching collections

$('a').click(function(){
    $('a').not(this).addClass('wow');
});

Cache please!

var anchors = $('a');
anchors.click(function(){
    anchors.not(this).addClass('wow');
});

That is, unless you have a good reason not to, e.g. new anchors are constantly being added to the document.

HTML

Long strings of HTML scarring your otherwise beautiful code. Keep the HTML… in the HTML, no in the JavaScript!

$('#something')
    .append('<div class="gall"><a href="javascript:void(0)">Linky</a></div>')
    .append('<div class="gall"><a href="javascript:void(0)">Linky</a></div>')
    .append('<button onclick="app.doStuff()">Button</button>');

I added a couple of other code smells (really bad ones!) in there. Please, oh please, learn how to use jQuery for your event handling!

Here’s a nicer smelling (and more readable) alternative:

var div = $('<div/>').addClass('gall').append(
    $('<a/>').click(function(){
        return false;
    })
);
 
$('#something')
    .append(div)
    .append(div.clone(true))
    .append(
        $('<button>Button</button>').click(function(){
            app.doStuff();
        })
    );

Leave a comment: Submit your own code smell! Or… tell me why I’m wrong!