What's that smell? That guy's code

Today I was going through my usual news feeds and was linked to an article about creating tabs that load in content via AJAX. I clicked the link and started reading the article and could not believe what I saw. This person was putting some of the least thought-out code I have seen in quite some time.

I am not going to link to this article because:

  • I don't want to spread this code around.
  • I don't think it is productive to call this person out.
  • This person doesn't claim to be a coder but an "internet entreprenuer" and "social media enthusiast" (really?!?).

So why bother doing this? Because I can't stand to see crappy or inefficient code! And, the only way to improve is to write (or rewrite) code. And, yes, I sent this person my final code. Hopefully, next time, they will take the time to learn something before they pollute the internet with their trash.

Anyway, here is the code that to me just reeks of bad practices. First the HTML:

  <div id="tabbed-posts">
      <header class="navlinks">
        <a href="#" id="newpostslink" class="active">Newest</a>
        <a href="#" id="popularpostslink">Popular</a>
        <a href="#" id="randompostslink">Random</a>
      </header>
  </div>

Now, the JavaScript:

$(function(){
    $('#tabbed-posts .navlinks a').on('click', function(e){
    e.preventDefault();

    if($(this).hasClass('active')) {
          return;
    } else {
          var currentitm = $('#tabbed-posts .navlinks a.active').attr('id');
          if(currentitm == 'newpostslink') { var currentlist = $('#tabbed-posts-new'); }
          if(currentitm == 'popularpostslink') { var currentlist = $('#tabbed-posts-popular'); }
          if(currentitm == 'randompostslink') { var currentlist = $('#tabbed-posts-random'); }

          var newitm = $(this).attr('id');
          if(newitm == 'newpostslink') { var newlist = $('#tabbed-posts-new'); }
          if(newitm == 'popularpostslink') { var newlist = $('#tabbed-posts-popular'); }
          if(newitm == 'randompostslink') { var newlist = $('#tabbed-posts-random'); }

          $('#tabbed-posts .navlinks a').removeClass('active');
          $(this).addClass('active');

          $(currentlist).fadeOut(320, function(){
            $(newlist).fadeIn(200);
          });
    } // end if/else logic

     }); // end event handler
});

So where to begin. Let's start with the JavaScript because, hey, it's my post and I want to...

Fixing the selector

The very first thing I noticed was the click event. What a crappy selector. Why is it crappy? Well, to begin with its an ID followed by a class followed by a tag. Thats a pretty complex selector. Nothing wrong with it if there isn't a better way. How about using the .find method instead? It's more efficient. Hell, there are at least three ways to make this more efficient. Don't take my word for it though. There is a jsperf that shows you everything you need to know. Check it out. I'll wait right here...

See what I mean? But even more than that, why do the other selections at all? Let's just put the click event on the tab container, let the event bubble up and capture the target of the event. Much easier; I like that. It's an ID so the selector is highly optimized to begin with.

var alltabs = $('#tabbed-posts'); // we will use this later on
alltabs.on('click', function( event ) {
    var $this = event.target;
    event.preventDefault();
    // code here ....
}

The worthless If

Next, lets take a look at that if statement.

if($(this).hasClass('active')) {
      return;
} else {
    // other code here ....
}

Hmmm.... if the link has a class of active go ahead and quit... Anyone else see the uselessness of this statement? Why do a check for something and then do nothing with it?!? Is it somehow required to test for a true value for doing something?!? Well, technically, yes, but we can also check for a negative to be true not just a positive. In other words, let's just check if it doesn't have the class.

if (! $this.hasClass('active') ) {
    // other code here ....
}

Well that wasn't too difficult was it? Now before we go on, I need to go on a side note here. You should never, ever have...

Links to nowhere

Why oh why does this person have links to nowhere?!? What is a link to nowhere? This is:

<a href="#">I don't go anywhere</a>

Yes, I understand you are going to load pages in via AJAX. I get it. But what about your non-JavaScript visitors? What if your ajax load fails? Don't you want them to be able to get to this other spectacular content? Don't you want SEO on those links? I'm guessing you do. Maybe I'm wrong, though. You are the "internet entrepreneur"....

This is a bad practice, people! Don't do it. Make a link go somewhere or open something! Always! That is why it is called a link! And that's why I love Revenge.css. It will call you out and show you how often you are being an asshole.

Besides, we can kill off a lot of that redundant crappy code if we can just read the href from the a tag. It also makes it easier to add additional tabs. Wow! We do it the right way and make it better at the same time. That's a win-win! We also shouldn't need that ID anymore. Win-win-win!

Updating the HTML

Of couse, I have no idea what the link really is, it isn't included in his code. Let's just make one up.

Maybe this is all being done on a single page via parameters. That would be smart. But let's assume this idiot has separate pages for each one. I will however assume that he is smart in one way... that all of that content is in its own container div and we only want to load that area, not the entire page. I assume this because it's pretty much a standard practice. That's a big assumption I know, but let's roll with it. I mean he is using AJAX so maybe he gets this part right.

Let's call our container div something real fancy like... ajaxcontainer and we will just append that div into our AJAX call. So in the first link below our AJAX call will load "newestposts.php #ajaxcontainer". We will also have one div on the page for each tab area. Let's add a data-area attribute to hold the id for each div and a data-state attribute on that new div to hold its current state. We will use this to check if we have already loaded the results via AJAX. Our HTML looks like this:

<a href="newestposts.php" data-area="newest">Newest</a>
<a href="popularposts.php" data-area="popular">Popular</a>
<a href="randomposts.php" data-area="random">Random</a>

<div id="newest" data-state="false">
    <!--newest posts get loaded via AJAX here -->
</div>
<div id="popular" data-state="false">
    <!--popular posts get loaded via AJAX here -->
</div>
<div id="random" data-state="false">
    <!--random posts get loaded via AJAX here -->    
</div>

What does that mean for our code? Well, all of the craptastic code below
gets replaced:

var currentitm = $('#tabbed-posts .navlinks a.active').attr('id');
if(currentitm == 'newpostslink') { var currentlist = $('#tabbed-posts-new'); }
if(currentitm == 'popularpostslink') { var currentlist = $('#tabbed-posts-popular'); }
if(currentitm == 'randompostslink') { var currentlist = $('#tabbed-posts-random'); }

var newitm = $(this).attr('id');
if(newitm == 'newpostslink') { var newlist = $('#tabbed-posts-new'); }
if(newitm == 'popularpostslink') { var newlist = $('#tabbed-posts-popular'); }
if(newitm == 'randompostslink') { var newlist = $('#tabbed-posts-random'); }

$('#tabbed-posts .navlinks a').removeClass('active');
$(this).addClass('active');

We will replace it with much more robust code that is DRYer and more performant. Personally I would use CSS animations to fade in and out, but for now this works.

var href = $this.getAttribute('href'), 
    area = $('#' + $this.getAttribute('data-area'));

alltabs.find('.active').removeClass('active').fadeOut(320, updateTabs);

function updateTabs() {
    if ( area.getAttribute('data-state') === 'false') {
        // AJAX-y stuff here and assuming success
        area.setAttribute( 'data-state' , 'loaded' );
    }
    area.addClass('active').fadeIn(200);
}

What do you know... a lot less douchey code. It is functionaly the same but more performant and a lot more DRY. So let's put all that together. Here is the code as it stands now:

$(function(){
    var alltabs = $('#tabbed-posts'),
        area, href;

    alltabs.on('click', function( event ) {
        var $this = event.target;  // let's cache our element
        event.preventDefault();
        if (! $this.hasClass('active') ) {
            var href = $this.getAttribute('href'), 
                area = $('#' + $this.getAttribute('data-area'));
            alltabs.find('.active').removeClass('active').fadeOut(320, updateTabs);

    function updateTabs() {
        if ( area.getAttribute('data-state') === 'false') {
            // AJAX-y stuff here and assuming success
            area.setAttribute( 'data-state' , 'loaded' );
        }
        area.addClass('active').fadeIn(200);
    }    
});

So... that works. I still don't like it, but it works. What don't I like? Well, if you know me, you know that I am not a big fan of polluting the global namespace and this code does just that. It's like a pretty girl wearing frumpy clothes. Also, It uses jQuery and it isn't straight JavaScript. I can live with that especially if you are using jQuery elsewhere. Let's at least take care of the first one. Lets put it into its own namespace.

var tabs = tabs || {

    alltabs : $('#tabbed-posts'),

    current : null,

    init : function () {
        tabs.alltabs.on('click', tabs.handler);
    },

    handler : function( event )  {
        var $this = event.target;

        event.preventDefault();
        if ( $this !== tabs.current ) {
            var href = $this.getAttribute('href'), 
                area = $('#' + $this.getAttribute('data-area'));

            alltabs.find('.active')
                .removeClass('active')
                .fadeOut(320);

            tabs.current = $this;
            tabs.update( area, href );
        }
    },

    update : function( area, href ){
        if ( area.getAttribute('data-state') === 'false') {
            // AJAX-y stuff here and assuming success
            area.setAttribute( 'data-state' , 'loaded' );
        }
        area.addClass('active').fadeIn(200);
    }
};

tabs.init();

Conclusion

She still might not be the prettiest belle at the ball, but she is no longer the wart-covered, buck-toothed, bow-legged, syphillis-infested crack-whore that she was. We could go a few steps farther like abstracting the CSS class names, adding in the "AJAX-y stuff" and doing some more tweaks here and there, but for now this is good enough.

If you see some bad code out in the wild, don't just accept it. Trash it, make fun of it, rub their nose in it, but clean it up! The web will be a better place for it.

'Til next time....