Code Review Feedback

[bundacia]

@jrpool: Here's some feedback. If you have specific questions let me know and maybe we can schedule some time to talk and walk through the relevant code together over zoom.

README

Sounds like you have a really good handle on how these solutions work. The README was easy enough for me to follow as I walked through the code. Your understanding of the execution order and scope seems spot on.

async-interval-N.js

I'm sure you're aware of this, but each of these solutions is an improvement on the one before it in terms of simplicity.

async-timeout.js

In async-timeout.js the recursive parameter isn't needed at all. If you remove it from nextIntegerOut and change:

recursive && console.log(current++);

to just:

console.log(current++);

things will still work great.

[jrpool]

Thank you for your detailed comments. They raise a couple of questions for me.

First, I generally understand why you say that the async-interval-N modules get simpler, although in one sense -3 is more complex than -2, namely in the complexity of the callback argument to setInterval. I wonder whether I'm alone in having a taste for defining named callbacks and using references to them as callback arguments, as being easier to humanly parse.

Second, I believe that the “recursive” parameter makes a difference, and it was described (perhaps obliquely) in the README file:

“The module treats the first iteration differently from those after it, in order to replicate the async-interval functionality, namely to output each integer after the specified delay, and after the last integer to exit with no delay.”

I have added an async-timeout-nr module without the “recursive” parameter to the repository (which I shall push in a minute). As you can see, the timing of the output of the first integer differs between it and the original version. Since the specs require interval and timeout implementations of the same functionality, I was aiming to reproduce the interval behavior, and the “recursive” parameter was one element of that solution. If you think there is a simpler way to achieve that, please let me know.

[bundacia]

Ahh, I see what you're saying about the recursive argument. You are correct. It is needing to get the timing right. I wonder if it could be eliminated if you moved the console.log into the setInterval callback like this:

const nextIntegerOut = (current, through, delay) => {
  if (current <= through) {
    setTimeout(
      () => {
        console.log(current++);
        nextIntegerOut(current, through, delay);
      },
      delay
    );
  }
};

As for your preference for named callbacks, I think often a named callback is indeed better, but in this case I like the the anonymous one since it's only a one line function that just calls another function, so it seems clear enough what it's doing without an explicit name.

[jrpool]

Thank you for your additional helpful thoughts. This comprehensive supervision is clearly more profound than what peer coaching provided under the former learning model.

Moving the console.log statement into the setTimeout function actually makes it worse (from the point of view of replicating the interval behavior). The missing delay before the first integer output is still missing. But now, in addition, a delay is inserted at the end after the last integer output, before the command prompt appears. This is verifiable with the -nri version that I have pushed to the repository.

[bundacia]

Huh, the delays seem correct to me, unless I'm assuming something incorrect about how this is supposed to work. With the console.log inside the setTimeout callback I get a delay before each number is printed (including the first) and no delay afterwards. Here's what it looks like when I run it: