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.

I'm working on an app that downloads some SVG paths from a server and draws them.

Anything in here is open to critique, but specifically I'd like to review my use of a module pattern and my implementation and use of promise objects.

The "modules" are each a file, and the aim is to limit and explicitly declare dependencies. My use of promises is to simplify the use of ajax.

My 3 files are:

  • init.js - the main application.
  • promise.js - my implementation of promises
  • ajax.js - ajax functionality

init.js:

(function (window, document, bigmap, ajax) {
    "use strict";

    var attr = {
        fill: "#fafafa",
        stroke: "#505050",
        "stroke-width": 1,
        "stroke-linejoin": "round"
    };

    var drawings, svg_canvas;

    svg_canvas = bigmap.initialize("map", window.innerWidth - 21, window.innerHeight - 21);

    ajax.post("php/paths.php", null)
        .then(function (value) { drawings = JSON.parse(value); })
        .then(function () { bigmap.draw(svg_canvas, drawings.walls, null); })
        .then(function () { bigmap.draw(svg_canvas, drawings.depts, attr); });

}(this, this.document, parent.bigmap, parent.ajax));

promise.js:

var promise = function () {
    "use strict";

    var pending = [],
        ready = false,
        result;

    var then = function (callback) {
        if (typeof callback === "function") {
            if (ready === false) {
                pending.push(callback);
            } else {
                callback(result);
            }
        }
        return this;
    };

    var keep = function (response) {
        if (ready === false) {
            ready = true;
            pending.forEach(function (value, index, ar) {
                result = response;
                value(result);
            });
            pending = undefined;
        }
    };

    var isReady = function () {
        return ready;
    };

    return {
        keep: keep,
        share: {
            then: then,
            isReady: isReady
        }
    };

};

ajax.js:

var ajax = (function (XMLHttpRequest, promise) {
    "use strict";

    var done = 4, ok = 200;

    function post(url, parameters) {

        var XHR = new XMLHttpRequest();
        var p = promise();

        if (parameters === false || parameters === null || parameters === undefined) {
            parameters = "";
        }

        XHR.open("post", url, true);
        XHR.setRequestHeader("content-type", "application/x-www-form-urlencoded");
        XHR.setRequestHeader('X-Requested-With', 'XMLHttpRequest');
        XHR.onreadystatechange = function () {
            if (XHR.readyState === done && XHR.status === ok) {
                p.keep(XHR.responseText);
            }
        };
        XHR.send(parameters);

        return p.share;
    }

    return {
        post: post
    };

}(parent.XMLHttpRequest, parent.promise));

Thanks.

share|improve this question
    
You shouldn't be using keywords like then as variable names. Most browsers will be ok with it, but it's by no means guaranteed. –  st-boost Nov 12 '12 at 21:18
3  
then is not a reserved word in javascript. –  Arjan Nov 13 '12 at 6:57
    
As for modules, I'd use RequireJS if I were you. –  ayanami Nov 13 '12 at 8:57

1 Answer 1

up vote 2 down vote accepted

I assume that you are not going to use readily available third-party libraries for this one. If I had the option to, I'd be using jQuery as it already supports promises, and AJAX.

Anyways, let's start reviewing. I pretty much explain in the comments so watch out for them. Any explanations are usually out of the comments though.

//actually, there is no point doing the localization thing since they 
//are objects (and passed by reference). Anything that changes the 
//original object, these references you have here also change
//
//However, it's still useful for assigning aliases so I'll let this one pass
(function (window, document, bigmap, ajax) {
    "use strict";

    //personal preference though
    //I prefer chaining the vars with commas
    //declarations on top, those with values last
    //also, you can merge declarations and assignments, like in svg_canvas
    var drawings, 
        svg_canvas = bigmap.initialize("map", window.innerWidth - 21, window.innerHeight - 21),
        attr = {
            fill: "#fafafa",
            stroke: "#505050",
            "stroke-width": 1,
            "stroke-linejoin": "round"
        };

    //promises are used for async to maintain a "sync" feeling in the code
    //however, if operations are not async, avoid using a promise
    //if bigmap.draw is not async, then I'd avoid chaining them in .then()'s
    //because it's overhead
    //
    //but I assume they are async since they are SVG drawing operations
    ajax.post("php/paths.php", null)
        .then(function (value) { 
            drawings = JSON.parse(value); 
        })
        .then(function () { 
            bigmap.draw(svg_canvas, drawings.walls, null); 
        })
        .then(function () { 
            bigmap.draw(svg_canvas, drawings.depts, attr); 
        });

}(this, this.document, parent.bigmap, parent.ajax));

On the promises, I'd avoid the module pattern and use prototypes. Imagine this: Every call to promise() returns an object with it's own properties as well as functions. When using prototypes, each promise will have it's own properties, but will share the functions. Forget privates, this one saves memory. However, just like me, if you hate the new Constructor() approach, you can mask the operation in a function call.

//constructor
function PromiseBuilder(){
    this.pending = [];
    this.ready = false;
    this.result;
}

//our shared functions

//this one stores the callback
PromiseBuilder.prototype.then = function (callback) {
    //personal preference, but I use single quotes
    if (typeof callback === 'function') {
        //ready is boolean. instead of checking for false,
        //you can use loose checking and ! which means *if not* ready
        if (!ready) {
            this.pending.push(callback);
        } else {
            //so you won't lose the instance, use call and pass this
            //as "this" in your function
            callback.call(this,result);
        }
    }
    //according to the PromisesA spec [http://wiki.commonjs.org/wiki/Promises/A]
    //each then should return a new promise
    //I haven't read the full spec or try implementing it
    //but you should, if you want it to become a proper PromiseA type promise
    return this;
};

//I guess this one acts as the "resolve" - you should name it that way
PromiseBuilder.prototype.resolve = function () {

    //saving the instance and all arguments
    var instance = this,
        resolveArgs = arguments;

    if (!ready) {

        ready = true;

        //ES5 foreach? or are you using a shim/fix?
        this.pending.forEach(function (value, index, ar) {

            //instead of being restrained in one argument,
            //why not pass the entire arguments array to the callback
            //by using .apply()
            value.apply(instance,resolveArgs);
        });

        this.pending = undefined;
    }
};

PromiseBuilder.prototype.isReady = function () {
    return this.ready;
};

//our masking function
function promise() {
    return new PromiseBuilder();   
};

Same as well for AJAX, we use literal namespacing instead. since the ajax response is now bound to the promise there is no need to have instances for our ajax.

var ajax = {
    post : function(url,parameters){
        var XHR = new XMLHttpRequest(),
            p = promise(),
            parameters = params || ''; //parameters is either itself or blank if none

        XHR.open("post", url, true);
        XHR.setRequestHeader("content-type", "application/x-www-form-urlencoded");
        XHR.setRequestHeader('X-Requested-With', 'XMLHttpRequest');
        XHR.onreadystatechange = function () {
            if (XHR.readyState === 4 && XHR.status === 200) {
                promise.keep(XHR.responseText);
            }
        };
        XHR.send(parameters);

        return promise;
    }
}
share|improve this answer
    
I'm not using 3rd party libraries because I want to understand everything that's going on while I learn. Also, the forEach I use is from ES5. –  ahuth Nov 18 '12 at 15:08
    
I've seen prototypes before, but I didn't realize that all instances of that object share the same functions (so you're not copying around function definitions that you don't have to). Also, that's a great idea of wrapping the new Constructor() in a function. I hadn't thought of that. –  ahuth Nov 18 '12 at 15:09
    
Finally, I wasn't aware that the then method is supposed to return a new promise. Does that mean that when you chain then calls together (promise1.then(something).then(something).then(something);) there are 3 separate promises, each with one callback in the pending array? And that if you don't chain them together (promise1.then(something); promise1.then(something); promise1.then(something);) you end up with one promise that has 3 callbacks in the pending array? Any idea what the benefit of this is? Either way, I'll go dig into the PromiseA spec. –  ahuth Nov 18 '12 at 15:15
1  
The idea of new promises was explained in this gist. And yes, methods attached to the prototype of an object is shared across instances, saving memory. However, properties should live in the instance. Shared properties is possible but discouraged as unexpected things may occur. –  Joseph the Dreamer Nov 19 '12 at 12:00
    
Thanks for the link to the gist. That's very helpful. If anyone else is looking for good links explaining promises implementation, here's another one. –  ahuth Nov 19 '12 at 13:52

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.