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.

In my never ending quest to write better code, I'm revisiting overall JS code structure and class patterns. I think I've come to a structure that I'm happy with, but would love to get some feedback from the community.

I'll include 4 JS code samples (global.js, class.Base.js, class.Overlay.js, and class.Modal.js) below, but here's a summary:

  • global.js handles setting up the namespace, a runner list (used to execute page load code in one DOM ready), list of available modules, etc. it's mainly just a wrapper json to hold all the pieces together.
  • class.Base.js sets up a base class that all other classes are derived from. It contains a set of methods that I would want on most other classes
  • class.Overlay.js is an example class that inherits from Base directly
  • class.Modal.js inherits from Overlay (and hence Base), overrides the "show" method

My initial goal was to do things the JS way. I've tinkered with (pseudo)classical inheritance to a certain extent, and I'm more in the camp of, why force JS to do something it wasn't meant to do?

So I wanted to use prototypal inheritance and because portability of the code is fairly important to me, I wanted a dynamic namespace. I also wanted private variables but I can rarely get everything I want.

In the end, my questions are, is there a way for me to use prototypal inheritance, and have real private variables (not a debate on whether or not I truly NEED privates, but it's something I'd like to know how to address if I ever decided I wanted/needed one)? And ultimately, is there any advice on the code samples below? Bad pattern? Better optimization?

Thanks in advance!

global.js

// ability to rename namespace easily
var AXS_NS = 'App';

// this is the only DOM ready event
// all other page load code is added to the runner
jQuery(function($){
    window[AXS_NS]._initialize();
});

window[AXS_NS] = (function (app, $, Modernizr) {
    // app.runList is used to run all init functions on a single dom ready
    app.runList = {};
    // app.modules keeps a list of all available modules
    app.modules = {};

    app._initialize = function() {
        this.$html = $('html').data(AXS_NS, this);
        this.run();
    };

    // executes all init functions attached to this.runList
    app.run = function() {
        $.each(this.runList, function(runItemName, json) {
            if (json.init && $.isFunction(json.init)) json.init();
        });
    };

    app.augment = function(json) {
        $.implement(json, app);
    };

    app.listen = function(evt, fn) {
        $(this).bind(evt, fn);
    };

    app.unlisten = function(evt) {
        $(this).unbind(evt);
    };

    app.dispatch = function(evt, args) {
        $(this).trigger(evt, args);
    };

    app.add = {
        "runItem": function(runItemName, json) {
            app.runList[runItemName] = json;
        }
        ,"module": function(moduleName, module) {
            app.modules[moduleName] = module;
            $.plugin(moduleName, module);
        }
    };

    app.remove = {
        "runItem": function(runItemName) {
            delete app.runList[runItemName];
        }
        ,"module": function(moduleName) {
            delete app.modules[moduleName];
        }
    };

    return app;

})(window[AXS_NS] || {}, jQuery, Modernizr);

class.Base.js:

// Base class used to derive ALL other classes
window[AXS_NS] = (function (app, $, Modernizr) {
    function Base () {};

    Base.prototype = {
        // takes options passed in and sets them as properties of the instance
        // e.g this.setProperties('$elem', 'name');
        "setProperties": function() {
            var self = this;

            $.each(arguments, function(k, prop) {
                if (self.cfg[prop]) self[prop] = self.cfg[prop];
            });
        }
        // update configuration json of the instance
        ,"updateCfg": function(options) {
            this.cfg = $.extend(true, this.cfg, options);
        }
        // creates a dom bridge between the instance and a dom element for later access
        // will only work if the instance has a property of $elem (obviously?)
        // try to keep the bridgeName the same as the instance Class name
        ,"domBridge": function(bridgeName) {
            this.$elem.data(bridgeName, this);
        }
        // shortcut for $.implement
        ,"augment": function(json) {
            $.implement(json, this);
        }
    };

    app.add.module('Base', Base);

    return app;

})(window[AXS_NS] || {}, jQuery, Modernizr);

class.Overlay.js:

// inherits from Base
window[AXS_NS] = (function (app, $, Modernizr) {
    function Overlay (options) {
        this.cfg = $.extend(true, {}, this.defaults, options);
        // set autoInit to false to manually initialize the class
        if (this.cfg.autoInit) this._initialize();
    };

    var prototype = {
        "defaults": {
            $elem: null, $target: null, width: 100, height: 50, autoShow: true, autoCenter: true, autoInit: true
        }
        ,"_initialize": function() {
            console.log('overlay._initialize()');
            this.setProperties('$elem');
            if (this.init) this.init();
        }
        ,"init": function() {
            console.log('overlay.init()');
        }
        ,"show": function() {
            console.log('overlay.show()');
            this.$elem.show();
        }
        ,"hide": function() {
            this.$elem.hide();
        }
    };

    // inherit from Base
    Overlay.prototype = $.extend(true, {}, app.modules.Base.prototype, prototype);

    app.add.module('Overlay', Overlay);

    return app;

})(window[AXS_NS] || {}, jQuery, Modernizr);

class.Modal.js:

// inherits from Overlay (which inherits from Base)
window[AXS_NS] = (function (app, $, Modernizr) {

    function Modal (options) {
        this.cfg = $.extend(true, {}, this.defaults, options);
        // set autoInit to false to manually initialize the class
        if (this.cfg.autoInit) this._initialize();
    };

    // create reference to parent (Overlay) show method
    var overlayShow = app.modules.Overlay.prototype.show;

    var prototype = {
        "defaults": {
            $elem: null, $target: null, width: 100, height: 50, autoShow: true, autoCenter: true, autoInit: true
        }
        ,"init": function() {
            console.log('modal.init()');
            this.setProperties('$elem');
        }
        // overrides app.modules.Overlay.prototype.show
        ,"show": function() {
            // use apply for proper this binding (if not, this = window in Overlay.prototype.show)
            overlayShow.apply(this);

            console.log('modal.show()');
            this.$elem.show();
        }
    };

    // inherit from Overlay
    Modal.prototype = $.extend(true, {}, app.modules.Overlay.prototype, prototype);

    app.add.module('Modal', Modal);

    return app;

})(window[AXS_NS] || {}, jQuery, Modernizr);
share|improve this question
add comment

migrated from stackoverflow.com Apr 5 '11 at 9:43

This question came from our site for professional and enthusiast programmers.

2 Answers

I personally go for a different pattern with an emphasis of hiding information from global state, being event driven and loading files only as and when they are needed. In your head you place.

// this is the only DOM ready event
jQuery(function() {
    load("main.js");
});

function load(fileName, callback( obj ) ) is a function that loads a file and takes a callback returning the object the file defined as a parameter.

Here's your main.js

// main.js
(function($, Modernizr) {
    var app = {},
        modules = {};

    // executes all modules.
    var run = function() {
        $.each(modules, function(key, val) {
            // feature load determines whether we want to load the module.
            if (featureLoad(key)) {
                load(val, function(module) {
                    module(app).start();
                });
            }
        });
    };

    app.augment = function(json) {
        // ???
        $.implement(json, app);
    };

    app.listen = function(evt, fn) {
        $(this).bind(evt, fn);
    };

    app.unlisten = function(evt) {
        $(this).unbind(evt);
    };

    app.dispatch = function(evt, args) {
        $(this).trigger(evt, args);
    };

    $.getJSON("modules.x", function(json) {
        modules = json;
        app.run();
    });

})(jQuery, Modernizr);

function featureLoad(string) Is a function that takes a string to identify a "feature" and returns true/false depending on whether that feature is needed on this page. The main page gets a list of features/files from the server as json and then runs those modules.

A module might be :

// a module.js file

(function($, Modernizr) {
    load("modal.js", function(Modal) {
        define(function(app) {
            var start = function() {
                var modal = Modal();

                modal.$elem = $("foo");
                modal.show();
            };

            return {
                "start": start   
            };
        });  
    });
}(jQuery, Modernizr));

function define(obj) defines the object that file will return when loaded through load.

Notice how a module is a function that takes app as an argument. This way app get's passed by reference and does not need to be global. It also returns an object with a start method so it's not automatically started. This particular module loads "modal.js"

// Modal.js
// inherits from Overlay (which inherits from Base)
(function($, Modernizr) {

    load("overlay.js", function(Overlay) {
        function Modal(options) {
            var overlay = Overlay(options),
                overlayShow = overlay.show;

            $.extend(true, this, overlay);

            var init = function() {
                console.log("modal.init()");
                // unneccesary second call
                this.extend("$elem");
            };

            var show = function() {
                overlayShow.apply(this);    

                console.log("modal.show()");
                // unneccesary second call.
                this.$elem.show();
            };

            $.extend(true, this, {
                "init": init,
                "show": show   
            });

            if (this.cfg.autoInit) {
                this.init();
            }
        }

        define(function(options) {
            return new Modal(options);    
        });
    });

})(jQuery, Modernizr);

modal.js defines a function that takes options and returns a new Modal object with those options. The modal object itself loads overlay and extends it into itself.

Notice how there are no defaults here because the defaults are already defined in overlay.js and as the options are passed into overlay we don't need to do this twice.

// overlay.js
// inherits from Base
(function($, Modernizr) {
    var defaults = {
        $elem: null,
        $target: null,
        width: 100,
        height: 50,
        autoShow: true,
        autoCenter: true,
        autoInit: true
    };

    load("base.js", function(Base) {
        function Overlay(options) {
            var base = Base();
            this.cfg = $.extend(true, {}, defaults, options);

            var init = function() {
                console.log("overlay.init");
                this.extend("$elem");
            };

            var show = function() {
                console.log("overlay.show");
                this.$elem.show();
            };

            var hide = function() {
                console.log("overlay.hide");
                this.$elem.hide();
            };

            $.extend(true, this, {
                "init": init,
                "show": show,
                "hide": hide
            });

            $.extend(true, this, base);

            // set autoInit to false to manually initialize the class
            if (cfg.autoInit) {
                this.init();
            }
        }

        define(function(options) {
            return new Overlay(options);
        });
    });

})(jQuery, Modernizr);

overlay.js works in a similar way but loads and extends Base. Since overlay sets it's options and defaults you don't need to repeat that in modal.js

// base.js
// Base class used to derive ALL other classes
(function($, Modernizr) {
    function Base() {
        var extend = function() {
            var self = this;
            $.each(arguments, function(key, val) {
                if (self.cfg[val]) {
                    self[val] = self.cfg[val];
                }
            });
        };

        $.extend(true, this, {
            "extend": extend
        });
    }

    define(function() {
        return new Base();
    });

})(jQuery, Modernizr);

base itself is quite simple. I've removed some clutter functionality that didn't seem useful.

Now you need to define featureLoad yourself. Preferably in a "featureLoad.js" file that's loaded through load. Many libraries have definitions of load and define. I personally use requireJS. which renames load to require and has define itself. If you use requireJS the syntax changes a little :

(function($, Modernizr) {
    define("modal.js", function(Modal) {
        return function(app) {
            var start = function() {
                var modal = Modal();

                modal.$elem = $("foo");
                modal.show();
            };

            return {
                "start": start   
            };
        };
    });  
}(jQuery, Modernizr));

Now this kind of method is just a different route to take and to me personally it embraces javascript as a functional language in its structuring. Notice how apart from external libraries like require & jQuery nothing is written to global scope.

I would recommend you read through this and see if you can find anything that you would like to adjust your own setup with.

All the code

My initial goal was to do things the JS way. I've tinkered with (pseudo)classical inheritance to a certain extent, and I'm more in the camp of, why force JS to do something it wasn't meant to do?

So I wanted to use prototypal inheritance and because portability of the code is fairly important to me, I wanted a dynamic namespace. I also wanted private variables but I can rarely get everything I want.

I completely agree with you. I personally find that the prototype is over-rated or I just don't know how to use it. So I use closures everywhere to handle scope and I extend objects dynamically to have inheritance through object composition.

Here rather then a dynamic namespace you have a self contained set of code that modularly loads various sub modules based on feature dependence. The entire code is encapsulated in private state which is passed around by functions. You also get private variables.

I have yet to find a good place to include prototypical inheritance into this code-base. If you do find the sweet spot in terms of balance let me know. I find that prototypical inheritance and closure encapsulated state just don't mix.

Also notice with my setup your are driven to seperate each class into an individual file and a module of functionality into a file. Each module of functionality will load the classes it needs and bind to the DOM how it wishes.

There is also no inline JS in your page apart from your initial $(function() { load("main.js"); } which can be avoided by using data-main attribute together with requireJS and wrapping your code in "main.js" in a $(document).ready() statement.

share|improve this answer
    
@Raynos lotta code to digest and pour over. Will respond to this, at a glance I like the thinking behind your approach. Thanks! –  alex heyd Apr 5 '11 at 0:46
    
@Raynos, my two first concerns would be the number of http requests and the inability to debug. I guess the http requests are made on demand, but if a certain module requires 3 classes, I rather have all 3 classes minified and combined with the rest of my JS served from the backend. As for debugging, do all the closures make firebug useless? I have come across certain situations in which I created classes etc and firebug would error but not with a helpful line number or anything. But I might implement the way you extend classes, by defining an instance of its parent –  alex heyd Apr 5 '11 at 2:48
    
@alex heyd the number of HTTP request is diverted by your standard code packager and compiler. You need to differentiate between code layout and what your packaging and minifying to your client. firebug does not give you useful information if you load files through eval rather then script injection. My personal standard method of debugging is using console.log rather then breakpoints so that doesn't matter too much to me. –  Raynos Apr 5 '11 at 7:02
    
Also if you split your code into small modules with many HTTP requests then you only load a minimal amount of code per page and since everything is in a small piece of re-useable code you get more caching then if you were to bundle them together. It's a trade-off. The inability to debug isn't the real issue, rather then trying to figure out in which particular closure your state is and ensure that this behaves. You need to be very comfortable with functional JS, to not introduce subtle bugs. –  Raynos Apr 5 '11 at 7:10
add comment

Well one thing, overwriting the namespace each time is unnecessary:

window[AXS_NS] = (function (app, $, Modernizr) { // don't set
    /*...*/
    app.add.module('Modal', Modal); // this will add to the object reference
    return app; // dont return
})(window[AXS_NS] || {}, jQuery, Modernizr);

As far as the private variables go with prototype objects, I worked up a small example. I'm sure there are other ways to do this - maybe someone else has a better idea. This would have to be extended out to destroy the private variables manually when the class is destroyed. Another option also besides using a name is to use an array and the length property for the identifying factor or a custom generated private key.

Look at this example:

(function(){
    var privates = {},
        privateType = function(){ return {foo: '_foo'}; };

    window['Class'] = function Class(name){
        this.name = name;
        privates[this.name] = privateType();
    };

    Class.prototype = {
       getFoo: function(){
           console.log(this.name + ' get: ' + privates[this.name].foo);
       },
       setFoo: function(value){
           privates[this.name].foo = value;
           console.log(this.name + ' set: ' + privates[this.name].foo);
       }
    };
})();

var c1 = new Class('c1');
c1.getFoo();        // get private
c1.setFoo('_foo1'); // set private
c1.getFoo();
share|improve this answer
    
@Josiah Good point, thanks. I think I had left it in there in case the JS files aren't loaded in the same order, that way the global namespace will be properly instantiated. But upon further thought, if I can't guarantee that global.js will be loaded before other JS files, I'd say I'm doing something else entirely wrong =P –  alex heyd Apr 4 '11 at 21:11
    
Interesting approach. I may be wrong, but in this case wouldn't the privates json be shared amongst all instances? I understand that this.name instructs it on which key to use in privates, but if let's say a class instance would have 10 private variables, and i have 3 instances, I'd essentially have 3 keys for each instance, then each would have 10 keys, one for each private var? Thanks for your time though! Much appreciated. –  alex heyd Apr 4 '11 at 21:51
    
Well more private variables would be added to this section return {foo: '_foo'}; change like return {private1: 'default value', private2: 'default value', etc: '...' };. privates[this.name] will always return the private instance for this particular class. Given that this.name is unique amongst all instances. In the fiddle example I created 3 different instances. –  Josiah Ruddell Apr 4 '11 at 21:58
    
Privates and prototypes don't go well together. Using pseudo privats in the format of this._private is better. Having to call privates[this.name].foo is messy. –  Raynos Apr 4 '11 at 22:09
    
@Raynos - I agree it is messy. But "pseudo privates" like this._private are just public with an underscore in front. –  Josiah Ruddell Apr 4 '11 at 22:14
show 1 more comment

Your Answer

 
discard

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