4
\$\begingroup\$

I'm working on my first NodeJS / Express application with the async module. It's been an uphill battle for me learning node (and even JavaScript) as I go along so I'm sure there's a lot of room for improvement.

I've written a GET event handler for my REST API and I'd like to know:

  • Where and how to decompose this into smaller methods
  • Areas where I can improve / simplify the logic
  • How I can make this more understandable

Most of my colleagues have never written JavaScript and do not understand async programming. I realize that they'll have to do a lot of their own homework to learn.. but I'm also trying my best to communicate intent as clearly as possible in the code with my method names and comments.

The challenge I've had is with async.map calls. I'm not too sure if it's a good idea to call other functions within the async.map callback function so right now, as you can see, I have a lot of logic within the callback.

router.get('/:widgetnum', function(req, res, next) {
        var widgetnum = req.params.widgetnum;
        var pairs = {};
        if ( !valid_widget(widgetnum) ) {
                res.status(400).send("Invalid Widget");
                return;
        } else {
                var keys = {};
                redis.hkeys("widget:" + widgetnum, function(err, data){
                        if (err) {
                                 winston.log('error', 'router.get() query to redis.hkeys() failed with err: ' + err);
                                 res.status(500).send("Failure while querying HKEYS");
                                 return;
                        }
                        console.log(data.length);
                        if (data) {
                                if (debug) {winston.log('info', 'router.get() query to redis.hkeys() returned : ' + data); }
                                current = getCurrentUTC(); // returns something like [ 1158, 't' ]
                                //create pairs with hash name and widget number, in addition to the results from HKEYS
                                pairs = data.map(function(ts) {
                                        return ['widget:'+widgetnum, ts];
                                });
                        }

                        if (data.length > 1) {
                                if (debug) { winston.log('info', 'router.get(widgetnum) multiple keys found in hash.  calling iterator') };
                                async.map(pairs, hash_iterator, function (err, iterator_results) { //async.map returns array of items. 
                                        if (err) {
                                                 winston.log('error', 'router.get() query to redis.hkeys() failed with err: ' + err);
                                                 res.status(500).send("Failure while querying HKEYS");
                                                 return;
                                        }
                                        if (debug) { winston.log('info', 'router.get() results from multi tc iterator query are ' + iterator_results); }
                                        var tc_uri = '';
                                        var default_uri = '';
                                        for (var i = 0; i < iterator_results.length; i++) {
                                                if (iterator_results[i] !== null) {
                                                        var tmp = iterator_results[i].split("~");
                                                        if (tmp[0].indexOf("00:00:00_00:00:00") != -1) {
                                                                default_uri = tmp[1];
                                                        } else {
                                                                tc_uri = tmp[1];
                                                        }
                                                }
                                        }
                                        if (tc_uri.length > 0 ) {
                                                res.send(JSON.stringify(tc_uri));
                                                return;
                                        } else {
                                                res.send(JSON.stringify(default_uri));
                                                return;
                                        }
                                });
                        } else if (data.length == 1) {
                                if (debug) { winston.log('info', 'router.get(widgetnum) only one key found.  calling default iterator'); }
                                async.map(pairs, default_iterator, function (err, iterator_results) {
                                        if (err) {
                                                winston.log('error', 'router.get() failure while trying to HGET default tc: ' + err);
                                                res.status(500).send("Unable to query database for default rule");
                                                return;
                                        }
                                       if (debug) { winston.log('info', 'router.get() results from default iterator query are ' + iterator_results); }
                                        var tmp = iterator_results[0].split("~");
                                       res.send(JSON.stringify(tmp[1]));
                                       return;
                                });
                        } else if (data.length == 0) {
                                if (debug) { winston.log('info', 'router.get() query to HKEYS for widget ' + widgetnum + ' returned nothing ' ); }
                                res.status(404).send("Unknown widget");
                                return;
                        }

                });
        }
});
\$\endgroup\$
2
\$\begingroup\$

I think some simple code clean-up would go a long way here. You have a number of nested conditionals that simply don't need to exist. For example:

    if ( !valid_widget(widgetnum) ) {
            res.status(400).send("Invalid Widget");
            return;
    } else {
            var keys = {};
            ...

There is no reason at all for your else condition, as you are returning in the previous conditional. That should just be:

    if ( !valid_widget(widgetnum) ) {
            res.status(400).send("Invalid Widget");
            return;
    }

    var keys = {};
    ...

I also don't understand the conditionals around the debug logging. I would think you would just always do the logging and just have it properly logged using .debug() or .log('debug', 'message') Conditionally logging in your code just means you are now adding your own log level concept outside the framework of the logger.


You can also probably reconsider your main conditional inside your redis.hkeys() callback. There seems significant commonality between your data.length === 1 and data.length > 1 use cases. Do you really need a different iterator and thus fork your code logic like this?


I don't understand why pairs is initialized as object literal when it is then overwritten by an array (mapped from data).


Why have if(data) in the hkeys() callback, when you don't do anything for the case where this evaluates as false? Furthermore, you still evaluate data in subsequent conditionals even if this evaluates as false. Perhaps consider checking if(!data) and failing out appropriately, so you can then move mapping to pairs out of a conditional altogether and reliably evaluate data.length in subsequent conditionals.

I do also question whether you should be evaluating against pairs.length instead of data.length since pairs truly becomes the primary subject of your logic after data has been mapped to pairs. I know this may seem like semantics, but you could technically make the memory allocated to data available for garbage collection after it is mapped, as it really no longer needs to be used. If data is large this might help you out, though I guess if data were large, you might have best iterated data and changed it in place rather than using map operation which would double memory utilization.


Maybe I am missing it, but I don't see where keys variable is used. You should remove if not used.


To your question on calling async.map against user-defined functions, I see no problem with this. I think this could reduce overall nesting level and make the logic around the Redis lookup read much more cleanly, without all the specific data transformation logic you have in the map callback.

I would would suggest that all this transformation should actually take place in the iteratee passed to async.map() as opposed to the the callback function that is passed as you are currently doing. This callback should really only handle error conditions or perform whatever action needs to happen on the result array upon completion of the mapping operation (in this case return a response to the caller).


A stylistic note, and maybe a result of copy-paste, But I think having eight spaces for each indentation is a little overkill and makes your code hard to read. I might suggest four spaces if you are not locked into this particular style for some reason.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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