Reduced nesting with async.waterfall but added clutter

Question

I am trying to reduce the nesting of async calls (node + socket.io) by using async.waterfall and I ended up having to append parameters down the waterfall because they are needed later. This code might explain better:

// Original version:

 socket event: turn action
  socket.on('turn action', function(gameId, turnAction, clientFn) {
    socket.get('corp', function(err, corp) {
      gameProvider.processTurnAction(gameId, corp.id, turnAction, function(err, msg, game) {
        clientFn(msg, game);
      });
    });
  });

// async.js version

  async.waterfall([
    function(callback) {
      socket.on('turn action', function(gameId, turnAction, clientFn) {        
        callback(null, gameId, turnAction, clientFn);
      });
    },
    function(gameId, turnAction, clientFn, callback) {
      socket.get('corp', function(err, corp) {
        callback(null, gameId, turnAction, clientFn, corp);
      });
    },
    function(gameId, turnAction, clientFn, corp, callback) {
      gameProvider.processTurnAction(gameId, corp.id, turnAction, function(err, msg, game) {
        clientFn(msg,game);
      });
    }
  ]);

The goal was readability but I find the redundant param passing adds clutter. I know that I can declare variables before the call to async.waterfall and store the params as needed for later use in the chain but that doesn't help with readability.

Is there a way to make this more elegant?

Cerelly

It looks elegant to me to be honest. – Kikuyu

Answer 1

I'm curious about the first function in your waterfall that sets up the turn action handler. Since it just specifies an event handler, it's technically synchronous (even though the handler itself will be called asynchronously). I'd probably refactor it thusly:

socket.on('turn action', function(gameId, turnAction, clientFn) {
  async.waterfall([
    function(callback) { socket.get('corp', callback); },
    function(corp, callback) {
      gameProvider.processTurnAction(gameId, corp.id, turnAction, callback);
    }
  ], function(err, msg, game) {
    // err will be set if either of the two `callback`s were called with
    // an error as the first parameter
    clientFn(msg, game);
  });
}

This has the added benefit of passing any error parameters into the final callback, so you can handle them as necessary (e.g. call clientFn with a parameter specifying an error).

Astronautics

This works like a charm! I was manually calling the callback per requirement noted in docs: tasks - An array of functions to run, each function is passed a callback it must call on completion. I did not realize that this was satisfied by the functions I was calling. waterfall seems much more useful to me now, thanks! – Cerelly

Yes, this is a pattern in JavaScript when the callback has the same arguments as the function you're calling; I talk about this more here: nodecasts.net/episodes/5-thinking-asynchronously – Astronautics

Actually I spoke too soon. It rolls to the next function when passing callback to the socket api but my own functions roll to the final optional callback as err. Looks fine in the debugger so I'm not sure why the param I am passing evaluates to error in the async.js code. – Cerelly

Are you making sure to pass null as the first parameter to the callback when there isn't an error? Async follows the Node.js pattern of callbacks having the arguments fn(error, others...), so you'd need to call something like callback(null, message, game). – Astronautics

Yup that seems to be the fix. I also bad a bad assumption on the behavior of socket.set which put my var out of scope. I had to declare a local var inside socket.on()'s callback to grab the reference to the var so that it can be used in the function following the call to socket.set. I might just do the socket.set() callback inline to avoid this and simply do "callback(myvar)". Also, thanks for the nodecast link. – Cerelly

Nice pattern - solved a similar problem for me. Thanks for posting. – Ectropion

@BrandonTilley, may I ask you to have a look at an async related question here - #27646535 ? – Encrust