Simple box with tabs

While trying to become better in javascript I've written the following simple box with tabs:

Javascript: (entire code (HTML/CSS) is in the snippet)

var Test = {};

Test.Tabs = function (element) {

    this.tabBox = element;

    this.init = function() {
        var tabs = this.tabBox.querySelectorAll('nav ul li a');
        for(var i = 0; i < tabs.length; i++) {
            tabs[i].addEventListener(
                'click',
                (function (event) {
                    if(event.ctrlKey || event.metaKey) {
                        return;
                    }
                    event.preventDefault();
                    event.stopPropagation();
                    this.toggle();
                }).bind(this),
                false
            );
        }
    };

    this.toggle = function() {
        this.hideAll();
        this.toggleClass(event.target.getAttribute('href'));
        this.show(event.target.getAttribute('href'));
    };

    this.hideAll = function() {
        var targets = this.tabBox.querySelectorAll('[id*=tab-]');
        for(var i = 0; i < targets.length; i++) {
            targets[i].style.display = 'none';
        }
    };

    this.toggleClass = function(id) {
        this.tabBox.querySelectorAll('.active')[0].classList.remove('active');
        this.tabBox.querySelectorAll('[href="'+id+'"]')[0].parentNode.classList.add('active');
    };

    this.show = function(id) {
        this.tabBox.querySelectorAll(id)[0].style.display = 'block';
    };

    this.init();
};

(function() {
    Array.prototype.forEach.call(
        document.querySelectorAll('.tabs-box'),
        function (element, index) {
            new Test.Tabs(element);
        }
    );
})();

var Test = {};

Test.Tabs = function (element) {

this.tabBox = element;

this.init = function() {
var tabs = this.tabBox.querySelectorAll('nav ul li a');
for(var i = 0; i
.tabs-box {
padding: 15px;
border: 1px solid #AFAFAF;
}

.ta

Your questions

• Do I need more properties so it would become better? (With better I mean performance and code maintainability. Like in the init() I could save all tabs and targets into properties, but then how to use them...)

Performance can come from simplifying. Don't duplicate variables or functionality. Dont define functions that are only called once. Less is always better when programming.

Maintainability. The best code is written once and never ever touched again. Code that has a focus on Maintainability is code that is unfinished and untested.

Make your code readable, use correct names that reflect the state/s it
represents, and keep to a consistent style. Test it thoroughly with the aim to make it crash. If you can not break it, it's finished.

• Not sure how object-oriented this is. So how to make it fully object oriented?

You use objects so I guess its object-oriented. OO is a very loose term, some would argue javascript is not an OO language, other say it is. Personally I can not wait for OOP to disappear as an abused concept and more specific terms used like inheritance, encapsulation, modularity... and more.

• Any thoughts in naming variables, functions and properties are welcome.

See below

Notes on changes made.

-
"use strict"; Use it always..

-
Declare variables and then functions at the top of the function. Also (not applicable in your code as you did not break this rule) for variables define/assign type/values only when needed and never where they are declared

-
You are using window.event which is a very non standard object introduced by IE. Its behaviour can not be trusted and should not be used in a multi browser environment.

-
Renamed Tabs to tab as it only handles a single tab.

-
When you created your tab object you passed the argument element. Then you assign that element to the property tabBox this is just duplication and not needed the argument is closed over for all the functions in the tab object.

-
AS you have not indicated a need for any type of interface (nor do you keep a referance to the object you create) there is no need to make its functions public. A safer approch is to protect all of its functionality by using closure.

-
Too many functions for very minor amounts of code. This is inefficient. You create functions when you have code that is repeated, or need to provide specific interface functionality. So I have collapsed all the functions into two toggle(ref) and tabClick(event)

-
You were creating a list of event handlers for each tab in a function init() The function is called only once during the instantiation. I could not see any reason for this to be a function. I moved it to inline.

-
There is a rule in javascript "DON'T create functions inside conditional blocks" loop blocks are conditional blocks. You were creating the anon event handler inside the loop that was iterating the tab elements. I moved the event handler out as a named function tabClick

-
You used the variable id for the page referance link. Id is something else so I considered it as a bad name choice. I have called it ref

-
Immediately invoked function are used to close over variables, or to create local scoped variables. If you are not closing over variables or creating any variables there is no need to use a immediately invoked function

-
Don't use inline styles. When you hide and show the various elements for the tabs and content you should use a CSS defined rule rather than a inline rule. I did not change that as I don't want to touch the CSS

Notes.

I would redesign the way tab works by having it encapsulate all of the tabs. For each instance of tab you end up iterating all the tabs so why isolate them to begin with.

The this token should only be used to expose the interface. If there is no interface (such as your code) then there should be no reason to use this to referance any internal functionality or state.

Code rewrite

Some additional comments can be found in the code.



"use strict"; // Never write code without this directive.
var Test = {};
// lower case tabs as it is no longer created with the new token
// It represents a tab not a group of tabs
Test.tab = function (tabBox) { // renamed argument from element to tabBox
var tabs, i; // variables to the top
function tabClick(event) {
if (event.ctrlKey || event.metaKey) {
return;
}
event.preventDefault();
event.stopPropagation();
// href returns full path so split at the # and get the last item
toggle("#" + this.href
.split("#")
.pop()
);
}
function toggle(ref) {
var targets, i; // declare variables at top of function
targets = tabBox.querySelectorAll('[id*=tab-]');
for (i = 0; i
.tabs-box {
padding: 15px;
border: 1px solid #AFAFAF;
}

.tabs-box nav ul {
list-style: none;
padding: 0;
margin

StackExchange Code Review Q#146672, answer score: 2