HTTP routing framework

Question

I am new to JavaScript programming and would like to get your comments/input or corrections on this chunk of code. I have tried to emulate patterns from recognized GitHub projects but I still don't have good feel for what "good" NodeJS code looks like.

I have written a very simple HTTP routing framework that works differently than ones currently available (it's a personal project / learning experience).

GitHub

var req, meth, url;
/**
 * Fork -> Synchronous process
 * @param  {Function} callback
 */
var _sync = function(callback) {
    _assembleTask(null, workFlow[meth][url].sync, function(err, tasks) {
        if (err) return callback(true, 'Invalide Workflow : ' + tasks);
        async.series(tasks, function(err, results) {
            return callback(err, results);
        });
    });
};

/**
 * Fork -> Parallel process
 * @param  {Function} callback
 */
var _async = function(callback) {
    _assembleTask(null, workFlow[meth][url].async, function(err, tasks) {
        if (err) return callback(true, 'Invalide Workflow : ' + task);
        async.parallel(tasks, function(err, results) {
            return callback(err, results);
        });

    });
};


/**
 * Merg - Parallel process
 * @param  {Object}   data - return values of sync and async
 * @param  {Function} callback
 */
var _merg = function(data, callback) {
    _assembleTask(data, workFlow[meth][url].merg, function(err, tasks) {
        if (err) return callback(true, 'Invalide Workflow : ' + tasks);
        async.series(tasks, function(err, results) {

            return callback(err, {
                'sync': data.sync,
                'async': data.async,
                'merg': results
            });
        });
    });
};

/**
 * URL Matcher
 * @param {String}   needle - incoming url
 * @param {Object}   haystack - JSON object for
 * @param {Function} callback
 */
var _urlMatch = function(needle, haystack, callback) {
    var match = null;
    for (var i in haystack) {
        if (urlglob(i, req.url)) {
            match = i;
            break;
        }
    }
    return callback(match);
};


/**
 * Assemble tasks
 * @param {data}     data - data to be passed into instantiated object
 * @param {taskList} taskList - list of tasks to instantiate
 * @param {Function} callback
 */
var _assembleTask = function(data, taskList, callback) {
    var tasks = {};
    var err = false;
    for (var key in taskList) {
        var module = taskList[key].split('.');
        if (!procsObj[module[0]]) return callback(true, module);
        var procsInst = new procsObj[module[0]](req, data);
        if (!procsInst[module[1]]) return callback(true, module);
        tasks[module[0] + '_' + module[1]] = procsInst[module[1]];
    }
    return callback(err, tasks);
};

/**
 * Main Method
 * @param  {Object}   request - request object passed from HTTP
 * @param  {Function} callback
 */
this.run = function(request, callback) {
    req = request;
    if (!(req.method in workFlow) && !('*' in workFlow)) {
        callback(405, 'Method Not Allowed');
    } else {
        meth = (req.method in workFlow) ? req.method : '*';
        _urlMatch(req.url, workFlow[meth], function(match) {
            if (match === null) {
                callback(404, 'Not Found');
            } else {
                var a = {};
                url = match;
                typeof workFlow[meth][url].sync != 'undefined' && (a.sync = _sync);
                typeof workFlow[meth][url].async != 'undefined' && (a.async = _async);
                async.parallel(a, function(err, results) {

                    typeof workFlow[meth][url].merg != 'undefined' && _merg(results, callback);
                    typeof workFlow[meth][url].merg == 'undefined' && callback(err, results);

                });
            }
        });
    }
};
};

Answer 1

I don't know what workFlow and urlglob are. So there might be wways to improve it that I don't know about.

_sync, _async and _merg are so similar that you should probably combine them in some way, instead of maintaining 3 near-identical copies of the same code. The callback part at the end is different in _merg because you're constructing an object at the end, but you can rewrite it so that the object-construction bit is handled where _merg is called.

In _urlMatch, the callback function doesn't follow the convention - the first argument should be an error argument. Even though you never need to call it with an error.

In fact, there's no reason why _urlMatch needs to be asynchronous at all. I.e. it shouldn't have the callback argument, and should finish with return match; instead of return callback(match);. Then, instead of _urlMatch(req.url, workFlow[meth], function(match) { ... });, do var match = _urlMatch(req.url, workFlow[meth]); ....

Similarly, _assembleTask can be made synchronous. (It makes a list of things to do; the things are asynchronous, but it doesn't actually do them, so it's OK.) The synchronous equivalent of callbacks with errors is the try-catch mechanism.

You've got a variable called module. nodejs has a built-in global variable called module. Since you've re-declared it, you're not interacting with the original module, so it's OK. Indeed, the global module usually isn't used inside function calls. But still, overriding variable declarations is generally frowned upon.

In _assembleTask, err is an unnecessary variable.

In _sync and _async, you can replace function(err, results) { return callback(err, results); } with callback.

You don't need any of these global variables (req, meth, url). You're only using them to pass information to _sync, _async, _merg and _assembleTask, so you should use arguments instead. It might look a bit messier, but global variables make code less modular and therefore harder to work with. Perhaps you did this because you didn't know what to write instead of a.sync = _sync, etc. You can use async.apply for this. I haven't documented the extra arguments to _sync/_async/_merg because I'm not sure exactly what they are.

More generally, you can use closures for this - so instead of async.apply(_sync, request, meth, match) in my version of the code, you could have written function(callback) { _sync(request, meth, match, callback); }. The anonymous function has access to the variables in its enclosing scope: request, meth, match. I only mention this because it's a more flexible way of doing things than async.apply. In this case it's not necessary, since you're writing _sync and can arrange for these three arguments to appear first.

You can replace typeof workFlow[meth][url].merg != 'undefined' with workFlow[meth][url].merg !== undefined. In fact, you can probably just replace it with workFlow[meth][url].merg. It depends on what values workFlow[meth][url].merg can have, so I can't be sure since you haven't told us what workFlow is. If workFlow[meth][url].merg is false, null, 0 or '', then workFlow[meth][url].merg is falsy but workFlow[meth][url].merg !== undefined is true; otherwise, it's OK to just use workFlow[meth][url].merg.

I would say you can do the same with typeof workFlow[meth][url].merg != 'undefined', but there's no reason why they should both be there anyway - just use if-else.

You seem to have an extra curly bracket at the end.

I've incorporated most but not all of these changes below. I haven't tested it, partly because I'm too lazy but mostly because I don't know what the setup is with these variables you haven't explained.

/**
 * Fork -> Synchronous process
 * @param  {Function} callback
 */
var _sync = function(req, meth, url, callback) {
    try {
        var tasks = _assembleTask(req, null, workFlow[meth][url].sync);
    } catch (mod) {
        return callback(true, 'Invalide Workflow : ' + mod);
    }
    async.series(tasks, callback);
};

/**
 * Fork -> Parallel process
 * @param  {Function} callback
 */
var _async = function(req, meth, url, callback) {
    try {
        var tasks = _assembleTask(req, null, workFlow[meth][url].async);
    } catch (mod) {
        return callback(true, 'Invalide Workflow : ' + mod);
    }
    async.parallel(tasks, callback);
};


/**
 * Merg - Parallel process
 * @param  {Object}   data - return values of sync and async
 * @param  {Function} callback
 */
var _merg = function(req, meth, url, data, callback) {
    try {
        var tasks = _assembleTask(req, data, workFlow[meth][url].merg);
    } catch (mod) {
        return callback(true, 'Invalide Workflow : ' + mod);
    }
    async.series(tasks, callback);
    });
};

/**
 * URL Matcher
 * @param {String}   needle - incoming url
 * @param {Object}   haystack - JSON object for
 */
var _urlMatch = function(needle, haystack) {
    for (var i in haystack) {
        if (urlglob(i, needle)) {
            return i;
        }
    }
    return null;
};


/**
 * Assemble tasks
 * @param {data}     data - data to be passed into instantiated object
 * @param {taskList} taskList - list of tasks to instantiate
 */
var _assembleTask = function(req, data, taskList) {
    var tasks = {};
    for (var key in taskList) {
        var mod = taskList[key].split('.');
        if (!procsObj[mod[0]]) throw mod;
        var procsInst = new procsObj[mod[0]](req, data);
        if (!procsInst[mod[1]]) throw mod;
        tasks[mod[0] + '_' + mod[1]] = procsInst[mod[1]];
    }
    return tasks;
};

/**
 * Main Method
 * @param  {Object}   request - request object passed from HTTP
 * @param  {Function} callback
 */
this.run = function(request, callback) {
    if (!(request.method in workFlow) && !('*' in workFlow)) {
        callback(405, 'Method Not Allowed');
    } else {
        var meth = (request.method in workFlow) ? request.method : '*';
        var match = _urlMatch(request.url, workFlow[meth]);
        if (match === null) {
            callback(404, 'Not Found');
        } else {
            var a = {};
            workFlow[meth][match].sync !== undefined && (a.sync = async.apply(_sync, request, meth, match));
            workFlow[meth][match].async !== undefined && (a.async = async.apply(_async, request, meth, match));
            async.parallel(a, function(err, results) {

                if (workFlow[meth][match].merg !== undefined) {
                    _merg(request, meth, match, results, function(err, merg) {
                        callback(err, {
                            sync: results.sync,
                            async: results.async,
                            merg: merg
                        });
                    });
                } else {
                    callback(err, results);
                }

            });
        }
    }
};