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?

It looks elegant to me to be honest. – freakish Sep 02 '12 at 21:51 — freakish, Sep 02 '12 at 21:51

score 5 · Accepted Answer · answered Sep 02 '12 at 22:09

5

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).

answered Sep 02 '12 at 22:09

Michelle Tilley

157,729
40
374
311

1

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! – wtjones Sep 03 '12 at 01:05
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: http://nodecasts.net/episodes/5-thinking-asynchronously – Michelle Tilley Sep 03 '12 at 01:24
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. – wtjones Sep 03 '12 at 02:52
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)`. – Michelle Tilley Sep 03 '12 at 03:01
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. – wtjones Sep 03 '12 at 03:09
Nice pattern - solved a similar problem for me. Thanks for posting. – BumbleGee Dec 22 '12 at 21:39
@BrandonTilley, may I ask you to have a look at an `async` related question here - http://stackoverflow.com/questions/27646035/node-js-express-executing-mysql-queries-one-after-another-within-loops-in-a-s ? – Istiaque Ahmed Dec 26 '14 at 15:33

Reduced nesting with async.waterfall but added clutter

1 Answers1