6
\$\begingroup\$

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");
        });
    });
});
\$\endgroup\$
3
  • \$\begingroup\$ In box.loadScript you can reduce to var urls = [].concat(options.url||options.urls||[]) and you save the line below. \$\endgroup\$
    – elclanrs
    Feb 21, 2014 at 10:53
  • \$\begingroup\$ 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). \$\endgroup\$
    – steros
    Feb 21, 2014 at 15:46
  • \$\begingroup\$ What you may and may not do after receiving answers. I've rolled back Rev 6 → 5. \$\endgroup\$ Oct 9, 2014 at 9:46

1 Answer 1

4
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ 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. \$\endgroup\$
    – steros
    Feb 26, 2014 at 11:41
  • \$\begingroup\$ 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. \$\endgroup\$
    – steros
    Feb 26, 2014 at 11:41
  • \$\begingroup\$ 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? \$\endgroup\$
    – steros
    Feb 26, 2014 at 11:44
  • \$\begingroup\$ Update question. \$\endgroup\$
    – konijn
    Feb 26, 2014 at 13:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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