HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

JavaScript Sandbox Pattern

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
javascriptsandboxpattern

Problem

Based on this Stack Overflow question I came up with the following code. I'd like your general thoughts about it. Code style, functional, logical, disadvantages, just anything you can come up based on the code that should be mentioned about it and springs into your eyes.

This is the sandbox:

function Sandbox() {
    // turning arguments into an array
    var args = Array.prototype.slice.call(arguments),
    // the last argument is the callback
    callback = args.pop(),
    // modules can be passed as an array or as individual parameters
    modules = (args[0] && "string" === typeof args[0]) ? args : args[0], i;

    // make sure the function is called
    // as a constructor
    if (!(this instanceof Sandbox)) {
        return new Sandbox(modules, args[1], callback);
    }

    // if supported use old values
    if (undefined != args[1] && "object" === typeof args[1]) {
        this.loadedScripts = args[1].loadedScripts;
        this.eventHandlers = args[1].eventHandlers;
    }

    // now add modules to the core 'this' object
    // no modules or "*" both mean "use all modules"
    if (!modules || '*' === modules) {
        modules = [];
        for (i in Sandbox.modules) {
            if (Sandbox.modules.hasOwnProperty(i)) {
                modules.push(i);
            }
        }
    }

    // always load internals
    Sandbox.modules['internal'](this);

    // initialize the required modules
    for (i = 0; i  0) {
            for (key in box.eventHandlers) {
                if (box.eventHandlers.hasOwnProperty(key) && /^0$|^[1-9]\d*$/.test(key) && key <= 4294967294) {
                    box.eventHandlers[key].off(); // remove event handler
                    box.eventHandlers.splice(key, 1); // remove from array
                }
            }
        }
    };
};


Here a sample module ("comment.js"):

```
Sandbox.modules.comment = function(box) {
var box = box || {};
box.loadScript({
url : "/js/template.js"
});

box.addComm

Solution

From a reverse once over:

  • $(this).attr('id') -> could be this.id



  • The click handler should not have to know where the path is of the js file



  • The click handler should not have to deal with the failure of loading the js file



  • The var statement in addComment.js is crazy, new lines are good for you



  • Same goes your var statement in comment.js



  • Using o and f as part of your API seems bad form, even for a Spartan coder like myself



  • if (-99 == response.header.code)



  • key



  • Using for (key in box.eventHandlers) is bad form when box.eventHandlers is an array, just use the good old for loop then you probably wont even have to check for hasOwnProperty`



All in all, I am not sure what you are trying to achieve with this pattern. You can download JavaScripts in an asynchronous fashion now. This will provide a much better user experience than lazy loading.

Edit:

  • RE:Edge, opening a connection to the server to download less than 1kb of JavaScript will appear slower than just loading 1 extra kb of code ( I am assuming that you minify your code agressively ) upfront, especially on Edge. Loading it asynchronously means that the page will render before all the js is loaded and gives the impression of speed.



  • Using booleans instead of -99 -> good idea



  • Eclipse and var statements -> Maybe change editor :P



-
As for the click handler, it should call an intermediate function that

  • Knows where the js files are ("js/app/")



  • Deals with the failure of loading the script



Otherwise every click handler has to know the path and has to deal with script loading failure.

Context

StackExchange Code Review Q#42407, answer score: 4

Revisions (0)

No revisions yet.