Application that creates a toolbar to show/hide elements on a page

Question

I've been spending a lot of time reading about JavaScript lately, and this is my first piece code that I've tried to apply some of what I've read to. I'm trying to stay clear of using a framework for this so I can have a better understanding of the JavaScript language itself.

This code is part of a chrome extention I wrote to add a toolbar on the bottom of a users GitHub dashboard so they can filter out items.

I'd like to get some feedback on its correctness, if there any best practices I've missed, and if there are any areas where I can be more "functional". Obviously I'd like to know if there are any obvious errors as well.

var filterObj = (function(){
    var filterObj,
        newsItems = [], 
        hiddenClasses = [],
        moreLink,
        visibleCount = 30,
        filterObjects = [],
        filters,
        versionKey = "githubNewsFilterVersion",
        filterKey = "filters";

    //private

    var getFilters = function(){
        //check for filters in the local storage, otherwise create a new object  
        if(!localStorage[filterKey]){
            filters = {
                issueComment : {text: "Issue Comment",id: "issues_comment",checked: false},
                pullRequest : {text: "Pull Request & Issue Opened",id: "issues_opened",checked: false},
                follow : {text: "Follow",id: "follow",checked: false},
                gist : {text: "Gist",id: "gist",checked: false},
                push : {text: "Push",id: "push",checked: false},
                created : {text: "Created Branch", id:"create",checked: false},
                issueClosed : {text: "Close & Merge", id:"issues_closed",checked: false},
                fork: {text: "Forked", id: "fork",checked: false},
                watch: {text: "Watch", id: "watch_started",checked: false},
                editWiki : {text: "Wiki", id: "gollum",checked: false}
            };

            localStorage[filterKey] = JSON.stringify(filters);
        }
        else{
            filters = JSON.parse(localStorage[filterKey]);
        }
    };

    var getNewsItems = function(callback){
        var items = getElementsByClass("div","alert"),
            len = items.length,
            newsLength = newsItems.length,
            found = false,
            currentItem = "";

        for(var i = 0; i < len; i++){

            found = false;
            currentItem = items[i];
            //check that the items isn't in the list
            for(var x = 0; x < newsLength; x++) {
                if(newsItems[x] == currentItem){
                    found = true;
                    break;
                }
            }

            if(!found){
                newsItems.push(items[i]);
            }
        }

        if(callback){
            callback();
        }
    };

    var createDiv =  function(){
            var newDiv = document.createElement("div");
            newDiv.id = "filterDiv";
            newDiv.className = "filterBar";

            document.getElementById("footer").appendChild(newDiv);

            filterObj = newDiv;

            createImg();
    };

    var createImg = function(){
        var closeSpan = document.createElement("span"),
            closeImage = document.createElement("img");

        closeImage.src = chrome.extension.getURL("assets/close.png");
        closeSpan.className = "closeBtn";

        closeImage.addEventListener("click",function(){
            document.getElementById("filterDiv").style.display = "none";    
        });

        closeSpan.appendChild(closeImage);
        filterObj.appendChild(closeSpan);
    };

    var createElement = function(theType, theID, theName, theValue, theAttrs, theClass){
        var newElem = document.createElement(theType),
            prop; 

        newElem.id = theID || "";
        newElem.name = theName || "";
        newElem.value = theValue || "";
        newElem.className = theClass || "";

        for(prop in theAttrs){
            if(theAttrs.hasOwnProperty(prop)){
                newElem[prop] = theAttrs[prop];
            }
        }

        return newElem;
    };

    var setFilters = function(){
        var prop; 

        for(prop in filters){

            var newFilterOption = createElement("input",
                                                prop,
                                                prop,
                                                filters[prop].id,
                                                {type : "checkbox"});
            addListener(newFilterOption);

            if(filters[prop].checked){
                newFilterOption.checked = true;
            }

            var newFilterLabel = createElement("label");
            newFilterLabel.innerHTML = filters[prop].text;  

            var newFilterWrapper = createElement("span");

            newFilterWrapper.className = "filterOption";
            newFilterWrapper.appendChild(newFilterLabel);
            newFilterWrapper.appendChild(newFilterOption);

            filterObjects.push(newFilterOption);

            filterObj.appendChild(newFilterWrapper);
        }
    };

    var addListener = function(elem){
        elem.addEventListener("change",function(){

            var newsObjects = newsItems,
                len = newsObjects.length,
                i;

            if(elem.checked === true){
                //loop through the elements array instead
                for(i = 0; i < len; i++){
                    if(hasClass(newsObjects[i], elem.value)){
                        newsObjects[i].style.display = "none";  
                        --visibleCount;
                    }
                }
            }
            else{
                for(i = 0; i < len; i++){
                    if(hasClass(newsObjects[i], elem.value)){
                        newsObjects[i].style.display = "inherit";   
                        ++visibleCount;
                    }
                }   
            }

            setFilterVal(elem.value,elem.checked);
        }); 
    };

    var setFilterVal = function(className,isChecked){

        for(filterObj in filters){

            if(filters[filterObj].id === className){
                filters[filterObj]["checked"] = isChecked;
                break;
            }
        }

        localStorage[filterKey] = JSON.stringify(filters);
    };

    var getMoreLink = function(){
        var moreDiv = getElementsByClass('div',"ajax_paginate")[0];
        moreLink = moreDiv.firstChild;

        attachClickListener();
    };

    var attachClickListener = function(){
        moreLink.addEventListener("click",function(){
                var i = 0;                          

                var intervalID = window.setInterval(function(){
                    i++;

                    getNewsItems(runFilters);

                    if( i === 20 ){
                        //reattach the event
                        getMoreLink();
                        window.clearInterval(intervalID);
                    }

                },200);
        });
    }

    var runFilters = function(){
        var len = filterObjects.length;

        for(var i = 0; i < len; i++){

            if(filterObjects[i].checked === true){

                var evt = document.createEvent("HTMLEvents");
                evt.initEvent("change", false, true);
                filterObjects[i].dispatchEvent(evt);
            }
        }

    }

    //looks like this function and the function below it can be rolled up into a partial
    //application
    var getElementsByClass = function(startTag, theClass){

        //if no start tag was specified then get all the elements
        var elements = startTag ? document.getElementsByTagName(startTag) : document.all;

        var matches = [];
        var pattern = new RegExp("(^| )" + theClass + "( |$)");

        for(var i =0; i< elements.length; i++){
            if(pattern.test(elements[i].className)){
                matches.push(elements[i]);
            }
        }

        return matches;
    };

    var hasClass = function(elem, theClass){

        var pattern = new RegExp("(^| )" + theClass + "( |$)"); 
        return pattern.test(elem.className);
    };

    var checkVersion = function(){
        var storedVersion = localStorage[versionKey],
            currentVersion = manifest.version;

        if(!storedVersion || storedVersion !== currentVersion){

            localStorage.removeItem(filterKey);
            localStorage[versionKey] = currentVersion;


        }
    };

    var manifest = (function() {
        var manifestObject = false;
        var xhr = new XMLHttpRequest();

        xhr.onreadystatechange = function() {
            if (xhr.readyState == 4) {
                manifestObject = JSON.parse(xhr.responseText);
            }
        };
        xhr.open("GET", chrome.extension.getURL('/manifest.json'), false);

        try {
            xhr.send();
        } catch(e) {

        }
        return manifestObject;
    })();

    //public
    return {
        Init : function(){
            checkVersion();
            getFilters();
            createDiv();
            getNewsItems();
            setFilters();
            getMoreLink();
            runFilters();
        }
    };  
}());

filterObj.Init();

Answer 1

Personally I wouldn't use the pattern

var functionName = function (params, etc) { 
};

as this will make any debugging a PITA. The call stack will show something like:

Anonymous method: line 12
Anonymous method: line 4

etc etc.

Since you already have it inside just do:

(function(){
    function functionName ( params, etc){
    }
})());

and you should be right to go. http://jsfiddle.net/6V26R/

As a personal preference I wouldn't do:

var filterObj = (function(){
    var filterObj,
        //etc

as I find our brains cannot read that so well. Instead I would do:

var filterObj = (function(){
    var filterObjElement,
        //etc

now my brain distinguishes between the global object and the local variable holding an element.

Answer 2

From a best practices standpoint, more comments will go a long way. Also the use of consistent camelCase is good and appreciated. One good practice which is pretty easy to implement is to declare all your variables at the start of the scope / function.

Answer 3

I'm going to post some of the feedback I got from the jsmentors group on google. This is the kind of stuff I was looking for.

From Nathan Sweet:

1. There is a structural issue in the way you organize you're code. You expose two global variables when you don't need to, not that it matters that much in this case. You are using a simple run-the-code-once scenario, which I personally think warrants the use of an anonymous object, but an anonymous function works well. Still, there is no need to initialize your code outside the function, in fact if everything was a function declaration you could initialize above all your functions. This is what an anonymous object looks like: ({ Init :function(){/run your code/}, method1:function(){this.method2();}, method2:function(){/does something/} }).Init()

2. Just being nitpicky, you've built a getElementsByClass method, and I'm not entirely sure what it does, but I see you're checking the individual className of the elements in the method. Chrome has a built in method for getting elements by class.

3. Lines 66-81 are painful, and I think you know it. Instead of looping through every element, try giving all the elements a unique id, or something like that, so that you can hash the results of one of those arrays and create an O(n) loop instead of an O(n*n) pattern.

From Austin Cheney:

This is the assignment way of creating a function: var whatever = function () {};

This is the declaration way of creating a function: function whatever () {}

The second one is hoisted, which is often convenient and rarely a problem. However, in those exotic instances when hoisting is a problem it is catastrophic. There is nothing you can do to prevent the var keyword from being hoisted, but you can prevent functions from being hoisted. The only exception about hoisting and declared functions is when the function is immediately invoked just like the "filterObj" function in your code. This is because the function executes immediately in its current form and position.

The common argument in favor of the declaration method, which I believe Rob was implying, is that for some people the declaration method is easier to read because you can quickly identify what is or is not a function. This is most certainly valid and I even agree with this fully for smaller applications. For larger applications containing many functions this argument fails. What is the value in trying to identify what is or is not a function if you have a container with 60 functions? At that point it is significantly faster to read the code by looking for the identifier and then determining whether the identifier is or is not a function by reading the very next word to its right.

I also find that the declarative way is more challenging to maintain in an extremely large application because it is not tied to the var keyword. I find that binding references, whether variables or function names, to a single var keyword dramatically reduces complexity of actually reading the code. Because of this I always recommend using no more than one var keyword per function and ensuring that nothing comes before this one var keyword except immediately invoked functions and the "use strict" pragma. This one variable rule when used with the "use strict" pragma results in generally more sturdy and portable code as well as code that is easier to read.

I see that around line 298 you are using a try/catch block. I absolutely detest try/catch blocks as a cheap attempt to mitigate known bugs. If you are aware of the possibility of a bug then correct your code. It the bug is the result of user input then output a response to this effect so that your user will know why the executed failed to perform as expected.

I also do not see the value in using a return on an anonymous object literal at the end of your application. Clearly this goes to the architecture of your application in that you want a series of sub-global functions to execute in a particular order that may or may not return anything but none the less result in a the global "filterObj" returning some object literal. This is an old convention, particularly the use of something named "init" that strings together a series of unrelated executions. Part of the reason you are probably using this convention is that all the functions in your application appear to exist in an equal scope directly under the global "filterObj" contain, and this is inefficient. Only create functions in the scope where they are needed, or if some functions are needed in different locations then at the minimum possible scope for reuse. Doing this decreases lookups, which dramatically increases execution speed. Speed in JavaScript really comes down to reducing lookups and using the most appropriate operator or method for a given job.

You also have some minor syntax violations in your code. For instance the "attachClickListener" is missing a terminating semicolon. This would prevent a bug free minification of your code. I would suggestion applying the prior mentioned guidance first and then after submitting your code through the JSLint tool.

From Fyodorov "bga" Alexander:

1) convert your code to

 {    const singletonObject = (function(){ 
     // helper fns 
     return  { 
       // privare members 
       newsItems_: [], 
       _init: function(){ 
         // your object init here 
         delete this._init // prevent double init 
         return  this 
       }, 
       // methods 
       getFilters: function(){ 
       } 
     }._init()    })()  } 

2) replace {!localStogare[foo]} to {localStogare[foo] != null}
3) put 1 extra space after comma in fn ivoke code { _fn(a, b) }
4) you have monster fn {createElement}. Too many args. Plz use cfg object. {createElement('div', {id: 'foo'})}
5) you can forget about semicolon 6) try to use fn style in js maximally, not

{getElementsByClass('div', 'foo')},    { 
     el.getElementsByTag('div')._map(function(v){ return 
 v.getElementsByClass('foo') })._reduce(function(v1, v2){ return 
 v1.concat(v2) })    }

7) use {localStogare.getItem}, not {localStogare[]}
8) alloc vars in place where you use it. For example> in {getNewsItems} you alloc {found} in top, but must in 68 line
9) you use direct DOM building using {document.createElement}, use > templates, its more readable and maintainable than set of {appendChild} and {createElement} ) 10) in lines 71 - 75 you have > {Array.prototype.indexOf}
11) its good that you use rule 1 var = 1 line

The full thread can be found here.

Answer 4

Why not continue your initial comma separated var declaration through to your functions? Like so:

versionKey = "githubNewsFilterVersion",
filterKey = "filters",
getFilters = function(){
 ...
},
getNewsItems = function(callback){
 ...

And so on.