Optimize this jQuery tab controller further

Question

How do I optimize this code further? Goal is to edit "js-tab" in one place. This is what I tried to follow.

Full code here.

var ToggleTab = {
init: function() {
 this.tab = $(".js-tab");
 this.bindEvents();
},
bindEvents: function() {
 this.tab.on("click", this.toggleTab);
 //this.tab.on("click", $.proxy(this.toggleTab, this));
},
toggleTab: function(e) { 
 var showTab = $(this).data("tab-content");
 e.preventDefault();
 $(".js-tab").each(function() {
 if ($(this).data("tab-content") === showTab) {
 $(".js-tab-" + showTab).removeClass("hide"); 
 } else {
 $(".js-tab-" + $(this).data("tab-content")).addClass("hide");
 }
 });
}
};
$(function() {
 ToggleTab.init();
});

• Sean Reid: Optimise for what - speed, memory, source file size? And what would be an acceptable value at which point you'll stop optimising? Unless you can answer these, you're wasting your time.
• Greg Burghardt: An acceptable value is already in his question: " Goal is to edit "js-tab" in one place."
• Tito: Just like Greg said.. also I'd like to use this.tab (like in bindEvents) inside toggleTab but I would get "undefined". Another way to say is I'd like to define a bunch of properties on init and reference them within the methods below. Hopefully I'm making sense.

Answer 1

I've always been partial to writing JavaScript classes that take a root element:

function TabController() {
 // Maintain reference to "this" in event handlers
 this.toggleTab = this.toggleTab.bind(this);
}
TabController.prototype = {
 $element: null,
 constructor: TabController,
 init: function(element) {
 this.$element = $(element).on("click", ".js-tab", this.toggleTab);
 return this;
 },
 toggleTab: function(event) {
 var that = this,
 showTab = event.target.getAttribute("data-tab-content");
 event.preventDefault();
 this.$element.find(".js-tab").each(function() {
 var tabContent = this.getAttribute("data-tab-content");
 if (tabContent === showTab) {
 that.$element.find(".js-tab-" + showTab).removeClass("hide"); 
 } else {
 that.$element.find(".js-tab-" + tabContent).addClass("hide");
 }
 });
 }
};

Plus, I like to replace jQuery API calls that wrap cross browser native DOM methods -- e.g. $(foo).data("bar") most times can be replaced with element.getAttribute("data-bar").

And to use:

// using document.documentElement means you don't need a jQuery dom-ready event handler
var tabs = new TabController().init(document.documentElement);

While you technically only need one tabber per page, making this an instantiable class allows you to unit test your JavaScript.

describe("TabController", function() {
 var element, tabs, event;
 function FauxEvent(type, target) {
 this.type = type;
 this.target = target;
 }
 FauxEvent.prototype.preventDefault = function() {};
 describe("toggleTab", function() {
 beforeEach(function() {
 element = document.createElement("div");
 tabs = new TabController().init(element);
 });
 it("shows a tab", function() {
 element.innerHTML = [
 '<a class="js-tab" data-tab-content="panel-1">Tab 1</a>',
 '<div class="js-tab-panel-1 hide"></div>'
 ].join("");
 event = new FauxEvent("click", element.firstChild);
 tabs.toggleTab(event);
 expect($(element.lastChild).hasClass("hide")).toBe(false);
 });
 });
});

This makes your code truly modular. You could go a step further and make the CSS class names and data attributes customizable:

function TabController() {
 // Maintain reference to "this" in event handlers
 this.toggleTab = this.toggleTab.bind(this);
 this.options = {
 classPrefix: "js-tab",
 hiddenClass: "hide",
 dataAttribute: "data-tab-content"
 };
}
TabController.prototype = {
 $element: null,
 options: null,
 constructor: TabController,
 init: function(element, options) {
 this.$element = $(element).on("click", "." + this.options.classPrefix, this.toggleTab);
 jQuery.extend(this.options, options || {});
 return this;
 },
 toggleTab: function(event) {
 var that = this,
 showTab = event.currentTarget.getAttribute(this.options.dataAttribute);
 event.preventDefault();
 this.$element.find("." + this.options.classPrefix).each(function() {
 var tabContent = this.getAttribute(that.options.dataAttribute);
 if (tabContent === showTab) {
 that.$element.find("." + that.options.classPrefix + "-"
 + showTab).removeClass(that.options.hiddenClass);
 } else {
 that.$element.find("." + that.options.classPrefix + "-"
 + tabContent).addClass(that.options.hiddenClass);
 }
 });
 }
};

Some sample HTML

<ul>
 <li class="js-tab" data-tab-content="content-a"><a href="#">Tab 1</a></li>
 <li class="js-tab" data-tab-content="content-b"><a href="#">Tab 2</a></li>
</ul>
<div class="js-tab-content-a">
 Content A
</div>
<div class="js-tab-content-b">
 Content B
</div>
<script type="text/javascript">
 var tabs = new TabController().init(document.documentElement);
</script>

JSFiddle: http://jsfiddle.net/CL8Ns/

• konijn: Very nice review, more detailed than mine, welcome to CodeReview. +1
• Tito: Hi Greg, this is very thorough and I really appreciated. A few things: 1)Is this some kind of common pattern out there that perhaps you can send me a link to check out with more documentation? 2) I noticed you use "new" to instantiate which I thought should be avoided according to Crockford?
• Greg Burghardt: I don't recall a website outlining this pattern. It's just a pattern I keep gravitating towards. Also, take Crockford's advice (and any Guru for that matter) with a grain of salt. The new keyword is not bad. He's just advising to not invoke object constructors unless you have a good reason to. Code like this is a good reason to, but notice the options property in my code is just using JSON instead of a JavaScript "class." That's the root of what Crockford is saying.
• Greg Burghardt: Another thought about the new keyword too. Most modern browsers have performance enhancements around object oriented code, including object constructor functions, that allow JavaScript to be executed at near native speeds. If you are doing new Foo() a thousand times inside a loop, yeah you might see a performance issue. Otherwise, you won't.
• Tito: @GregBurghardt - I tried your code but it's not quite working. Should it work or this is just an example of your pattern?

Answer 2

From a once over;

• You are selecting the content divs with a class name, you should select them with an id instead
• It is much more readable, to simply hide all content, and then show what you need.

With that I would counter propose the content to look like this:

<div class="js-tab-content" id='orange'>Orange Content</div>
<div class="js-tab-content hide" id='red'>Red Content</div>
<div class="js-tab-content hide" id='purple'>Purple Content</div>

and then toggleTab would be

toggleTab: function(e) { 
 var showTab = $(this).data("tab-content");
 e.preventDefault();
 $(".js-tab-content").hide();
 $('#' + showTab).show();
}

Otherwise I like your code, it's solid.