6 ms·

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.

by exit 14y ago

goblin89 14y 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 14y ago
  So you become a better programmer?
  - goblin89 14y 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 14y 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 14y ago
  Aren't code reviews normally meant to highlight which parts are good and which are bad?
  - goblin89 14y ago
    Not in this case, though—OP just explains what does the code do, without any opinion.
btipling 14y 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 14y 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 14y 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 14y 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 14y 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 14y 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...

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.

by exit 14y 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 14y 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 14y ago
So you become a better programmer?
- goblin89 14y 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 14y 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 14y ago
Aren't code reviews normally meant to highlight which parts are good and which are bad?
- goblin89 14y ago
  Not in this case, though—OP just explains what does the code do, without any opinion.

btipling 14y 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 14y 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 14y 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 14y 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 14y 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 14y 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...