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.

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 < modules.length; i += 1) {
        var module = modules[i];
        // load modules if not present
        if (typeof Sandbox.modules[module] == "undefined") {
            $.ajax({
                async : false,
                dataType : "script",
                cache : false,
                url : 'js//modules/' + module + '.js'
            });
        }
        Sandbox.modules[modules[i]](this);
    }

    // call the callback
    callback(this);

    // any prototype properties as needed
    Sandbox.prototype = {
        name : "Sandbox",
        version : "1.1",
        getName : function() {
            return this.name;
        }
    };
};

// set space for modules to add to the sandbox
Sandbox.modules = {};

Sandbox.modules.internal = function(box) {
    var box = box || {};
    box.eventHandlers = box.eventHandlers || [];
    box.loadedScripts = box.loadedScripts || [];

    box.loadScript = function(options) {
        var urls = options.urls || options.url || [];
        urls = $.isArray(urls) ? urls : [ urls ];
        $.each(urls, function(index, value) {
            if (-1 == $.inArray(value, box.loadedScripts)) {
                $.ajax({
                    async : false,
                    cache : false,
                    url : value,
                    success : function() {
                        box.loadedScripts.push(value);
                    }
                });
            }
        });
    };

    box.removeOldEventHandlers = function(options) {
        if (box.eventHandlers.length > 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.addComment = function(options) {
        var templateURL = options.templateURL || 'templates/comment/comment.js.tpl', commentsContainer = options.commentsContainer, trigger = options.trigger, text = options.text, guid = options.guid, newComment = $('<article style="display:none;"><img src="images/ajax-loader.gif" alt="loading comment" /></article>');

        newComment.prependTo(commentsContainer).show('fade', function() {
            $.post(SERVICE_URL, {
                o : 'comment',
                f : 'add',
                set : {
                    text : text,
                    id : id
                }
            }, function(response) {
                if (-99 == response.header.code) {
                    newComment.setTemplateURL(templateURL).processTemplate(response.content).fadeIn();
                } else {
                    box.buttonMessage({
                        message : response.header.userMessage,
                        button : trigger,
                        title : 'Error!'
                    });
                }
            }, 'json');
        });
    };
};

Here is the call to a modules function ("addComment.js"):

var Sandbox = Sandbox([ 'comment', 'message' ], Sandbox, function(box) {
    // add comment
    var trigger = $('#button_addComment'), comments = $('#comments'), commentsContainer = $('#commentsContainer', comments), input = $('#text', comments);
    box.eventHandlers.push(trigger);
    box.addComment({
        text : input.val(),
        guid : comments.attr('data-guid'),
        trigger : trigger,
        commentsContainer : commentsContainer
    });
});

And here the actual call to the code:

var SERVICE_URL = 'service.php';
$(function() {
    $('section#addComment').on('click', 'button.button', function(e) {
        e.preventDefault();
        var name= $(this).attr('id').split('_')[1];
        //getCachedScript is just the $.ajax equivalent of $.getScript but with caching
        // this would load "addComment.js"
        $.getCachedScript( "js/app/" + name + ".js" ).fail(function() {
            alert("This function is temporarily not available");
        });
    });
});
share|improve this question
    
In box.loadScript you can reduce to var urls = [].concat(options.url||options.urls||[]) and you save the line below. –  elclanrs Feb 21 at 10:53
    
Thanks! What I do not like so far is in "addComment.js". As you can see there are two modules loaded comment and message. But message is actually needed in the module comment for the function box.buttonMessage. So actually that module should be loaded in the module comment! But today I did not figure out how to do this. I guess I need to create a function for the code of loading modules in the Sandbox itself and make this function callable in a module to load (an)other module(s). –  4485670 Feb 21 at 15:46
add comment

1 Answer

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) <- Why ?
  • key <= 4294967294 <- This should be a commented constant
  • 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

    1. Knows where the js files are ("js/app/")
    2. 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.

share|improve this answer
    
Thanks for your answer! Actually I want to separate code and implement lazy loading to improve page speed. Why load all the magic™ if you never make a comment? I'm not sure how asynchronous loading would help in all cases. Imagine if you are on Edge on a mobile phone. –  4485670 Feb 26 at 11:41
    
For the click handler I think I either don't understand what you mean or I have no idea how to do it in another way. My var statement is usually chained but to different lines, to this day I could not find out how to tell eclipse not to push it back into one line. –  4485670 Feb 26 at 11:41
    
if(-99 == response.header.code) is a bit historic. code is -99 if everything is fine and a number bigger than zero if not (error codes). You are right maybe I should change it to if(true == response.header.success) and elseif(true == response.header.failure)? Something like that or do you have a better option? –  4485670 Feb 26 at 11:44
    
Update question. –  konijn Feb 26 at 13:54
add comment

Your Answer

 
discard

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

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