Problem
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.
Solution
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;
}
}