6 ms·

Analyzing mbostock's queue.js

110 points by bryansum 13y ago

goldfeld 13y ago
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.
geuis 13y ago
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 https://github.com/geuis/when-then
rlx0x 13y ago
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 http://vimeo.com/12529436
- Gmo 13y ago
  Hey thanks for that link, I learned a few nuggets in there ;)
exit 13y ago
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.
- goblin89 13y ago
  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?
  - atomical 13y ago
    So you become a better programmer?
    - goblin89 13y ago
      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.
      - atomical 13y ago
        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.
  - pekk 13y ago
    Aren't code reviews normally meant to highlight which parts are good and which are bad?
    - goblin89 13y ago
      Not in this case, though—OP just explains what does the code do, without any opinion.
- btipling 13y ago
  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.
  - xal 13y ago
    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.
  - wizard_2 13y ago
    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.
  - ufo 13y ago
    > 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.
- NelsonMinar 13y ago
  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-equidistant.js https://raw.github.com/mbostock/d3/master/src/geo/conic-equi...)
  - dribnet 13y ago
    > 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 https://github.com/mbostock/d3/wiki/Selections#wiki-attr [2] https://github.com/mbostock/d3/blob/v3.1.4/src/selection/attr.js#L58 https://github.com/mbostock/d3/blob/v3.1.4/src/selection/att...
aqrashik 13y ago
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-jquery-source/ http://paulirish.com/2010/10-things-i-learned-from-the-jquer... and should be interesting to people who liked this link
- mitchellhislop 13y ago
  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-jquery-source/ http://paulirish.com/2011/11-more-things-i-learned-from-the-...
mbostock 13y ago
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.
- bryansum 13y ago
  My pleasure. I figured this was a bit more manageable than doing d3.js at the moment ;).
- abecedarius 13y ago
  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.
  - mbostock 13y ago
    Using the results array to store the scheduled tasks is an interesting idea. Thanks for the suggestion!
    - abecedarius 13y ago
      You're welcome! I tried coding this up at https://gist.github.com/darius/5287542 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).

110 points by bryansum 13y ago

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.

geuis 13y ago

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 https://github.com/geuis/when-then

rlx0x 13y ago

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 http://vimeo.com/12529436

Gmo 13y ago
Hey thanks for that link, I learned a few nuggets in there ;)

exit 13y ago

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.

goblin89 13y ago
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?
- atomical 13y ago
  So you become a better programmer?
  - goblin89 13y ago
    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.
    - atomical 13y ago
      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.
- pekk 13y ago
  Aren't code reviews normally meant to highlight which parts are good and which are bad?
  - goblin89 13y ago
    Not in this case, though—OP just explains what does the code do, without any opinion.
btipling 13y ago
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.
- xal 13y ago
  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.
- wizard_2 13y ago
  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.
- ufo 13y ago
  > 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.
NelsonMinar 13y ago
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-equidistant.js https://raw.github.com/mbostock/d3/master/src/geo/conic-equi...)
- dribnet 13y ago
  > 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 https://github.com/mbostock/d3/wiki/Selections#wiki-attr [2] https://github.com/mbostock/d3/blob/v3.1.4/src/selection/attr.js#L58 https://github.com/mbostock/d3/blob/v3.1.4/src/selection/att...

aqrashik 13y ago

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-jquery-source/ http://paulirish.com/2010/10-things-i-learned-from-the-jquer... and should be interesting to people who liked this link

mitchellhislop 13y ago
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-jquery-source/ http://paulirish.com/2011/11-more-things-i-learned-from-the-...

mbostock 13y ago

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.

bryansum 13y ago
My pleasure. I figured this was a bit more manageable than doing d3.js at the moment ;).
abecedarius 13y ago
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.
- mbostock 13y ago
  Using the results array to store the scheduled tasks is an interesting idea. Thanks for the suggestion!
  - abecedarius 13y ago
    You're welcome! I tried coding this up at https://gist.github.com/darius/5287542 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).