— JavaScript, jQuery, Rant — 4 min read
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:
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:
1<div id="tabbed-posts">2<header class="navlinks">3 <a href="#" id="newpostslink" class="active">Newest</a>4 <a href="#" id="popularpostslink">Popular</a>5 <a href="#" id="randompostslink">Random</a>6</header>7</div>Now, the jQuery:
1$(function(){2 $('#tabbed-posts .navlinks a').on('click', function(e) {3 e.preventDefault();45 if($(this).hasClass('active')) {6 return;7 } else {8 var currentitm = $('#tabbed-posts .navlinks a.active').attr('id');9 if(currentitm == 'newpostslink') { var currentlist = $('#tabbed-posts-new'); }10 if(currentitm == 'popularpostslink') { var currentlist = $('#tabbed-posts-popular'); }11 if(currentitm == 'randompostslink') { var currentlist = $('#tabbed-posts-random'); }1213 var newitm = $(this).attr('id');14 if(newitm == 'newpostslink') { var newlist = $('#tabbed-posts-new'); }15 if(newitm == 'popularpostslink') { var newlist = $('#tabbed-posts-popular'); }16 if(newitm == 'randompostslink') { var newlist = $('#tabbed-posts-random'); }1718 $('#tabbed-posts .navlinks a').removeClass('active');19 $(this).addClass('active');2021 $(currentlist).fadeOut(320, function(){22 $(newlist).fadeIn(200);23 });24 } // end if/else logic2526 }); // end event handler27});So where to begin. Let's start with the jQuery because, hey, it's my post and I want to...
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.
1var alltabs = $('#tabbed-posts'); // we will use this later on2 alltabs.on('click', function( event ) {3 var $this = event.target;4 event.preventDefault();5 // code here ....6 }Next, lets take a look at that if statement.
1if($(this).hasClass('active')) {2 return;3} else {4 // other code here ....5}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.
1if (! $this.hasClass('active') ) {2 // other code here ....3}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...
Why oh why does this person have links to nowhere?!? What is a link to nowhere? This is:
1<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!
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:
1<a href="newestposts.php" data-area="newest">Newest</a>2<a href="popularposts.php" data-area="popular">Popular</a>3<a href="randomposts.php" data-area="random">Random</a>45<div id="newest" data-state="false">6 <!--newest posts get loaded via AJAX here -->7</div>8<div id="popular" data-state="false">9 <!--popular posts get loaded via AJAX here -->10</div>11<div id="random" data-state="false">12 <!--random posts get loaded via AJAX here -->13</div>What does that mean for our code? Well, all of the craptastic code below gets replaced:
1var currentitm = $('#tabbed-posts .navlinks a.active').attr('id');2if(currentitm == 'newpostslink') { var currentlist = $('#tabbed-posts-new'); }3if(currentitm == 'popularpostslink') { var currentlist = $('#tabbed-posts-popular'); }4if(currentitm == 'randompostslink') { var currentlist = $('#tabbed-posts-random'); }56var newitm = $(this).attr('id');7if(newitm == 'newpostslink') { var newlist = $('#tabbed-posts-new'); }8if(newitm == 'popularpostslink') { var newlist = $('#tabbed-posts-popular'); }9if(newitm == 'randompostslink') { var newlist = $('#tabbed-posts-random'); }1011$('#tabbed-posts .navlinks a').removeClass('active');12$(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.
1var href = $this.getAttribute('href'),2 area = $('#' + $this.getAttribute('data-area'));34alltabs.find('.active').removeClass('active').fadeOut(320, updateTabs);56function updateTabs() {7 if ( area.getAttribute('data-state') === 'false') {8 // AJAX-y stuff here and assuming success9 area.setAttribute( 'data-state' , 'loaded' );10 }11 area.addClass('active').fadeIn(200);12}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:
1$(function(){2 var alltabs = $('#tabbed-posts'),3 area, href;45 alltabs.on('click', function( event ) {6 var $this = event.target; // let's cache our element7 event.preventDefault();8 if (! $this.hasClass('active') ) {9 var href = $this.getAttribute('href'),10 area = $('#' + $this.getAttribute('data-area'));11 alltabs.find('.active').removeClass('active').fadeOut(320, updateTabs);1213 function updateTabs() {14 if ( area.getAttribute('data-state') === 'false') {15 // AJAX-y stuff here and assuming success16 area.setAttribute( 'data-state' , 'loaded' );17 }18 area.addClass('active').fadeIn(200);19 }20});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.
1var tabs = tabs || {23 alltabs : $('#tabbed-posts'),4 current : null,56 init : function () {7 tabs.alltabs.on('click', tabs.handler);8 },910 handler : function( event ) {11 var $this = event.target;12 event.preventDefault();13 if ( $this !== tabs.current ) {14 var href = $this.getAttribute('href'),15 area = $('#' + $this.getAttribute('data-area'));1617 alltabs.find('.active')18 .removeClass('active')19 .fadeOut(320);2021 tabs.current = $this;22 tabs.update( area, href );23 }24 },2526 update : function( area, href ){27 if ( area.getAttribute('data-state') === 'false') {28 // AJAX-y stuff here and assuming success29 area.setAttribute( 'data-state' , 'loaded' );30 }31 area.addClass('active').fadeIn(200);32 }33 };3435 tabs.init();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,
-G