Hacker News Reader

Comment by mbostock | Hacker News Reader

This is great! Thank you for the code review. One of the aspects of Google culture that I enjoyed the most were the asynchronous, detailed code reviews. Knowing that someone you look up to will pore over every line of code encourages you to get both the big picture and the details right. That happens with less regularity in open-source, although it’s been a pleasure working with brilliant people such as Jason Davies. So I appreciate this review greatly; it helps not just explain the code but encourages me to do better.

View on HN

A couple suggestions for this code:

- Couldn't it be simpler without the linked-list queue? Use an array: the first ndispatched are for calls that have been invoked; they each hold either the corresponding result or undefined if not yet resolved by their callback. The rest of the array holds arguments objects for pending calls. When it's time to notify, this array is the results array.

- The 2-space indent plus fairly deep nesting makes it hard to see where the 'main story' starts and ends, at least for me. 4-space indent goes better with this nested style.

Using the results array to store the scheduled tasks is an interesting idea. Thanks for the suggestion!

You're welcome! I tried coding this up at https://gist.github.com/darius/5287542 (untested). I see you have your own version now, which is longer but has a loop where I fell back to the tail call. (I also cut out a line in notify(), though you might prefer the old behavior of passing undefined for the results array on error, vs. the new one of an empty array. I guess I would prefer the old behavior on reflection.)

Some doc suggestions from my draft and I'll finally shut up:

    //  Can you call things after the first .await or .awaitAll?
    //  Whatever the answer, it should be documented.
    //  Maybe document the assumption that callbacks get invoked exactly once?
    //  (It'd be possible to ensure it's at most once.)
    //  Document assumption: the notified awaitAll function won't mess with its array argument
    //  (this matters if we're called again after).

Something else I'm unsure about: line 67, `results[i] = r`, refers from a closure back to i initialized at line 52 in a loop. If this loop pops more than one deferral, then won't each deferral's callback use the same i? Since Javascript assigns to function-level i in the loop, it doesn't bind a new one.

OTOH if the loop only ever pops at most one deferral, then an if instead of a while would make this clear.

Good catch! It appears I just introduced the bug here, by eliminating recursion: https://github.com/mbostock/queue/commit/7bd5059bb2c0a94c0ca...

Popping multiple synchronous tasks after an asynchronous one is fairly rare, but I’ll make sure the tests verify this code path.

My pleasure. I figured this was a bit more manageable than doing d3.js at the moment ;).

Reply on news.ycombinator.com