stephenwoods.net

I was speaking with my colleague Matt about refactoring Javascript. One of the most common “smells” in code, or at least in my code, is duplication. A really common one for me to see several event handlers or callback functions that all do the same thing, but in ever so slightly different cases. This came up most recently when I was hacking together a routing module for a node.js project.

Because js is functional I had the idea of routes actually being an array of functions that took a request object and returned a route object. Pretty powerful, you could have routing based on anything, not just urls. Anyway, what actually ended up happening was this:

var routes = [

    /* / */
    function(request){
        if(/^\/$/.test(url.parse(request.url).pathname)){
            return {
                controller:'index',
                action:'index',
                data:{}
            };
        }else{
            return false;
        }
    },

    /* /about */
    function(request){
        if(/^\/about$/.test(url.parse(request.url).pathname)){
            return {
                controller:'index',
                action:'about',
                data:{}
            };
        }else{
            return false;
        }
    },

    /* /news/story/[0-9] */
    function(request){
        var test = /^\/news\/story\/([0-9]+)$/
                    .exec(url.parse(request.url).pathname);

        if(test){
            return {
                controller:'news',
                action:'showStory',
                data:{
                    storyId:test[1]
                }
            };
        }else{
            return false;
        }
    }
];

As you can see, I’ve written the same function twice at the top, more or less. Now there are a couple of ways to refactor this. The obvious one of course is to have the function which acts on this array check if the input is an object or a function and act accordingly, so that the route could just be an object with a regex and info about the action and controller. I decided not to go with this option because I like to avoid code with lots of switching and cases if possible, just running the function seems much cleaner to me.

So instead I created a function that would create the most common case functions. That way I could write my routes like this:

var routes = [

    /* / */
    r(/^\/$/, 'index', 'index'),

    /* /about */
    r(/^\/about$/, 'index', 'about', {foo:'bar'}),

    /* /news/story/[0-9] */
    function(request){
        var test = /^\/news\/story\/([0-9]+)$/
                    .exec(url.parse(request.url).pathname);

        if(test){
            return {
                controller:'news',
                action:'showStory',
                data:{
                    storyId:test[1]
                }
            };
        }else{
            return false;
        }
    }
];

Much cleaner, just as flexible, and to the caller it looks exactly the same, because the function ‘r’ returns a function that looks like the one I created for the news story, like so (I also added the feature that query variables would be added to the data object):

function r(regex, controller, action, data){
    var exp = regex,
    d=data || false,
    ret = {
        controller:controller,
        action:action || 'index'
    };

    return function(request){
        var uri = url.parse(request.url, true);
        if(exp.test(uri.pathname)){

            //get the query string and add it to the data
            var retData = uri.query || {};
            if(d){
                for(var i in d){
                    retData[i] = d[i];
                }
            }

            ret.data = retData;

            return ret;

        }else{
            return false;
        }
    };
};