Analyzing mbostock's queue.js | Hacker News Reader

Analyzing mbostock's queue.js

bsumm.net

110 pointsbryansum26 comments

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.

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

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.

That's so great, and mbostock's a coder I greatly admire. I have had it on my mind for the past few weeks precisely this--to get into the habit of reading code, and in doing so write deep articles explaining it. Thanks for this.

Paul Irish's "10 Things I Learned From the jQuery Source" is also a really good analysis of the jQuery Source, particularly for people still getting started with Javascript.

It's available at http://paulirish.com/2010/10-things-i-learned-from-the-jquer... and should be interesting to people who liked this link

The followup is also fantastic: "11 More Things I Learned From the jQuery Source": http://paulirish.com/2011/11-more-things-i-learned-from-the-...

Thats a neat code review, it reminded me a little of the really awesome review of jquery, "10 things I learned from the jquery source": http://vimeo.com/12529436

Hey thanks for that link, I learned a few nuggets in there ;)

Interesting, I took a deep look at his code today too over lunch. I like seeing different people's approaches to this problem.

My own that I wrote a while back is called when-then, https://github.com/geuis/when-then

i can't decide whether this code is clever or borderline obfuscated. if i were a more confident programmer i'd go with the latter.

IMO any overly clever code would remind of obfuscation. The trick is to notice in time your own attempts at being smart and refrain. Thus the code could communicate most of the things that needed to be explained in this walkthrough.

That's also why I personally am not very enthusiastic about such code reviews. Good code would seem boring, and why study bad code?

So you become a better programmer?

Fair enough, maybe it would work for someone. I find that reading and especially writing code of certain style shapes my style accordingly, similar to how our environment influences us, so I'd rather learn by working with good code—but it's just me.

Code reviews are about finding (1) the proper way to do things (2) edge cases, bugs, and the right architecture. Code reviews are not academic at all. They do require someone with a lot of experience.

Aren't code reviews normally meant to highlight which parts are good and which are bad?

Not in this case, though—OP just explains what does the code do, without any opinion.

I'd say I like how the thing does what it does, and I even like the overall simplicity of each of the functions which seem easily testable. I just don't like the JavaScript very much.

> var node = arguments;

> node.i = results.push(undefined) - 1;

...

Adding properties to an arguments object, assigning the arguments object (which is not an Array, only array-like) to a variable, pushing undefined onto an array and subtracting by 1 to get the latest index.

This isn't clever, it's silly.

Simple is better than complicated. They're passing a special array-like object all over the place, note:

> slice.call(node, 1),

They do this because slice isn't on arguments. They are using an arguments outside of the function's scope.

The var statement in a while loop gives me the impression this person doesn't know how scope works in JavaScript.

It's also not a very good idea for performance. V8 optimizer will bail as soon as you do anything to arguments other then using an indexed access.

Mike Bostock is the author of at least three javascript visualization libraries that are in heavy use today. It's safe to say he's got a firm understanding of javascript. While including a var in side a loop doesn't sope the variables to the loop, it is convenient. You'll notice he's careful to assign to all of the variables with each iteration and doesn't use them outside the loop.

The "connivence" taken with some of the other parts of the code (particularly the complicated one liners) I'll raise an eyebrow over, but they show a deep understanding of javascript. However I'm not fond of conciseness over simplicity.

> The var statement in a while loop gives me the impression this person doesn't know how scope works in JavaScript.

I'm not the author of queue.js but I actually declaring my Javascript variables as close to their use point as possible because JSHint warnings let me pretend that JS had real block scope all along. You would be amazed how often it catches "variable used out of scope" errors that would not be caught if the declarations had been hoisted to the top of the function.

I often have this feeling when I read Mike's code, particularly D3. Then I read the code again and work to understand it and I learn something new myself. Mike's coding style leans to very terse; one letter variable names, fearless use of operator precedence and side effects. It requires you understand the language to read it clearly, but the advantage is a lot more code fits in a small space. His code is quite poetic, it's a nice change of pace from Java verbosity. (My favorite cosmetic idiom he uses: non-ASCII variable names for spherical trigonometry: https://raw.github.com/mbostock/d3/master/src/geo/conic-equi...)

> I often have this feeling when I read Mike's code, particularly D3.

Agreed - there is tons to learn in the D3 src, but I don't feel that it is written in a way friendly to casual inspection. But D3 more than makes up for this with such clear and thorough documentation that this is rarely an issue.

Random example: wondering what a d3 selection.attr returns? Reading the docs [1] is a much quicker read than scanning the source [2] and working backwards from this nested return one-liner:

    return value == null
        ? (name.local ? attrNullNS : attrNull) : (typeof value === "function"
        ? (name.local ? attrFunctionNS : attrFunction)
        : (name.local ? attrConstantNS : attrConstant));

[1] https://github.com/mbostock/d3/wiki/Selections#wiki-attr

[2] https://github.com/mbostock/d3/blob/v3.1.4/src/selection/att...

Reply on news.ycombinator.com