9
\$\begingroup\$

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

});
\$\endgroup\$
3
  • \$\begingroup\$ 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. \$\endgroup\$
    – Sean Reid
    Apr 22, 2014 at 10:03
  • \$\begingroup\$ An acceptable value is already in his question: " Goal is to edit "js-tab" in one place." \$\endgroup\$ Apr 22, 2014 at 13:04
  • \$\begingroup\$ 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. \$\endgroup\$
    – Tito
    Apr 22, 2014 at 16:18

2 Answers 2

12
\$\begingroup\$

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/

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

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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