Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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();

});
share|improve this question
    
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. –  Sean Reid Apr 22 at 10:03
    
An acceptable value is already in his question: " Goal is to edit "js-tab" in one place." –  Greg Burghardt Apr 22 at 13:04
    
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. –  Tito Apr 22 at 16:18

2 Answers 2

up vote 12 down vote accepted

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/

share|improve this answer
4  
Very nice review, more detailed than mine, welcome to CodeReview. +1 –  konijn Apr 21 at 19:32
    
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? –  Tito Apr 21 at 23:27
    
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 Apr 22 at 12:41
    
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. –  Greg Burghardt Apr 22 at 12:44
    
@GregBurghardt - I tried your code but it's not quite working. Should it work or this is just an example of your pattern? –  Tito Apr 22 at 16:30

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.