7 ms·

Code review without your glasses

175 points by padolsey 12y ago

swanson 12y ago
I've seen this type of analysis referred to as looking at the "shape of the code" and I personally like that metaphor. If you see any "clumps" or "spiky edges", that might be a sign to investigate further and see if the code is trying to tell you where the problems are. I find this technique useful when you are asked to review something but you don't have full context of the project/language/library - I could point out potential issues with e.g. a Go program, even though I've never written a single line of Go myself.
michaelfeathers 12y ago
Nice article but I think there's something missing. Some things are uniformly bad like multi-screen methods or methods with six levels of nested conditionals, but the shape of code isn't really uniform. You can't look at the class or method level and play the three bears game (this porridge is too hot, too cold, just right). In codebases, the ratio of big to small matters. It's fine to have some of those classes that seem too small. They are a natural consequence of good structuring. As well, it's okay to have a few large classes also, ones that you know could be broken down but not convincingly. The thing that's awful is that if you say this sort of thing people want to believe that it's license to allow classes and methods to grow without bound, but it isn't. If you aim for understandability, you'll have code with sizes that fall into a distribution, usually power-law-ish https://researchspace.auckland.ac.nz/bitstream/handle/2292/16834/Understanding%20the%20shape%20of%20Java%20software.pdf?sequence=2 https://researchspace.auckland.ac.nz/bitstream/handle/2292/1...
kylebrown 12y ago
> And whilst I can work out that that 20 line block in the middle is used to decide which users we need to send emails to if you give me an afternoon and some strong coffee, I would humbly suggest that you stick it inside a def users_to_send_emails_to so that I don’t have to. Love this part, seconded.
chrisweekly 12y ago
Thanks for sharing a good post. I think of these code "shapes" as decent examples of the more generic metaphor of code "smells". But sometimes you need more context / other senses to determine whether or not the choices at hand were pragmatic. For larger systems, tradeoffs involving local complexity vs global simplicity can't be judged in a vacuum.
ashmud 12y ago
When I read "without your eyes", I was thinking more of code analysis tools (of the "style" variant).
jonaldomo 12y ago
I enjoyed this post. I really thought this was going to be about using visual regression for code reviews. I do think you could probably automate some of this though which would make for an interesting framework!
saganus 12y ago
I'm not sure if I understood the article fully, so I would appreciate if someone could give me an explanation. From what I understood, and by reading the comments, it seems like there are certain patterns in code that could be somewhat-universally flawed? Like seeing a lot of indentation without meaningful code in between? Or am I getting this wrong? Is it really possible to establish a "score" of sorts for how the code looks without actually looking at what it does? If it is, then it would seem like an interesting idea to build a code "shape-analyzer" that would give you the same type of analysis. Or is this too hard to describe in terms of patterns that we can see but can't explain? I think I need my morning coffee... am I making any sense?
- Cthulhu_ 12y ago
  You are, but maybe the article isn't :p. I can identify with the post, but maybe a bit differently; when I read code, especially in a language I'm familiar with, I see patterns instead of individual lines of code. So when a piece of code breaks the patterns, or breaks a rhythm, or looks different from the other code, my spider sense tingles and I frown upon the code. There could be nothing wrong with it, but it looks different so I end up refactoring it until it fits the rest of the codebase and I understand it.
  - saganus 12y ago
    These patterns you speak of, are they on your own code or could you do the same with some one else's code? I ask this because I think I could do it with my own, but not sure if I could with another one's. Or maybe to the odebase of a particular product instead?
- goblin89 12y ago
  OP presents a few common anti-patterns in a humorous way. The shapes are how these anti-patterns might look in Ruby code, but anti-patterns themselves are language agnostic. Shapes are really just for illustration, but they also emphasize the irony: “you’re part of a preternaturally enlightened dev team” yet you review your coworkers' code without even reading. Perhaps there's something to be read between the lines. :)
Monkeyget 12y ago
You can do this more efficiently and for the entire code base at once by using code visualization tools: CodeCity [1]. A city represents a code base. Each class is represented as a building. The aspect of a building gives information on the corresponding class. For example: the color is the number of lines, the height is the number of methods and the width is the number of attributes. System Complexity View and Class Blueprint [2]. Like CodeCity the color and shape represent various attributes. An added benefit compared to the codecity is that you can also show links between the code elements. For example inheritance or invocation. NDepend [3] for .NET also has many interesting code diagrams. [1] http://www.inf.usi.ch/phd/wettel/codecity.html http://www.inf.usi.ch/phd/wettel/codecity.html [2] http://www.moosetechnology.org/docs/polymetricviews http://www.moosetechnology.org/docs/polymetricviews [3] http://www.ndepend.com/Features.aspx#Diagrams http://www.ndepend.com/Features.aspx#Diagrams
- saganus 12y ago
  That CodeCity idea is very very interesting.
  - jagtesh 12y ago
    Apparently, it's not just an idea: http://www.inf.usi.ch/phd/wettel/codecity.html http://www.inf.usi.ch/phd/wettel/codecity.html
    - saganus 12y ago
      True that. I meant not as an unrealized idea but as in generally, a good idea. I did check out the software but unfortunately it's not for commercial use. Still, neat way of analyzing code.
- anigbrowl 12y ago
  I don't understand why tools like this aren't built into every editor. Syntax highlighting and code folding are great, but those are ~30 year old innovations; adopting the 'blurry text' approach of the OP (where you can't read specific things), consider the general lack of visualization tools for utterly basic things like loops and branches. How come we don't automatically generate function flowcharts, for example? I use a lot of flow-based programming tools for DSP and the more time that goes by the more perverse the text fetish of many programmers appears, like a statistician that hates graphs and insists everything has to be presented in tables.
CaptainQuirk 12y ago
Excellent topic. It resembles something I've heard graphics designer do when they try to appreciate the information hierarchy or the contrast on a creation, without being disturbed by the meaning of words of symbolic value of graphical elements. Although, I disagree about comments. I mostly overcomment code, because sometimes, a project lacks a proper specification and it's sometimes the only place where to explain what you actually do.
nikisweeting 12y ago
Great article. I especially loved "to find out what the foo is going on". Does this mean I may gain some higher insight into my code by blurring my vision? I don't have any glasses to forget...
ckdarby 12y ago
After reading this I must ask you to please take the time to have a redundancy plan in place for your glasses; I recommend a secondary pair offsite at your work in order to prevent outages of lost/forgotten glasses.
hyperliner 12y ago
OP forgot that the main ingredient for a code review is TRUST. He should have simply told the dev that he forgot his glasses. Then asked if the dev would help a little. And yes, ckdarby's comment of getting a redundancy plan in place for your glasses is spot on.
emackn 12y ago
Similar to the Sandi Metz talk, All the Little Things, from rails conf. She talks about the "squint test" around the 5 minute mark. http://www.confreaks.com/videos/3358-railsconf-all-the-little-things http://www.confreaks.com/videos/3358-railsconf-all-the-littl...
- angersock 12y ago
  Wonderful talk. Really funny is the bit about "Yeah, when I get called in, it's not to be like, hey, look at this wonderful code!"
  - mjcohen 12y ago
    I agree. I really enjoyed her talk. It filled my head with a number of interesting ideas, some of which I might even use. "Small is beautiful."
- girvo 12y ago
  That was an amazing talk that really made me understand why SOLID and small objects are worth the initial hesitancy. Thanks so much for linking it, I think the people at work are going to love it.
DougWebb 12y ago
The article seems to be saying "I can't see anything you've written but I'm going to review it anyway: no matter what you've done, I don't like it." Maybe it's just the way the article is organized, or maybe I gave up reading the "I don't like this" examples before I got to one that said "This is good".
- fryguy 12y ago
  The not being able to see anything is just being clever. The article is really about bad designs that are identifiable by their structure alone. It's obviously going to primarily talk about "bad" ones because those are the interesting points.
JasonFruit 12y ago
It seems like Jeff Atwood wrote something similar to this several years ago, where he recommended pasting your code into Microsoft Word and dropping the font size to illegibility so you could see the patterns of indentation without being distracted by reading the code. I remember it being about equally valuable, without the undertone of snark I get from this article.
tjr 12y ago
Exhibit 5 Clarificatory comments, thumbs up! Code that requires an accompanying multi-chapter essay to understand, thumbs down. Looks like about 150 words of comments there, not a multi-chapter essay. That aside, I don't know what this code is doing, but working on sundry avionics software I would find that approximate amount of commentary very helpful in some situations. I would be remiss to downplay the value of this commentary without reading the actual words.
- yellowapple 12y ago
  Same here. And even if that amount of commenting isn't necessary, I'll take excessive commenting over insufficient commenting any day.
  - m_mueller 12y ago
    Depends on the comments. IMO comments together with the code should still follow the DRY principle - since code states exactly what it does, the comments should only state why it does what it does - except if we're talking about comments intended to be exposed to the documentation. Otherwise comments become outdated very quickly, ensuing confusion further down the line.
    - wiredfool 12y ago
      The most valuable comments that I've found are documentation as to exactly why the code is doing something that looks strange or wrong, on purpose. It keeps maintainers from going in and breaking stuff later. Yes tests. But people can 'fix' tests at the same time too.
- Argorak 12y ago
  Except one, all those comments are on top of methods and the class. This can quite possibly be interface documentation. For a public API, this wouldn't be a lot of docs.
- patrickg 12y ago
  Agree. I have some complex code that I revsit once in a month, and I cannot afford thinking though it again and again. Therefore my comments are getting larger and larger and more chapter like with many images inbetween. (I use locco for that, see http://speedata.github.io/luaqrcode/docs/qrencode.html http://speedata.github.io/luaqrcode/docs/qrencode.html for a different but similar example.)
jacalata 12y ago
You’re part of a preternaturally enlightened dev team, and have set aside an entire day for nothing but code review. However, after the first 2 hours, you realise that you have forgotten your glasses and have just been staring at colourful blurs all morning. What do you do? You go talk to your boss and tell them you are taking indefinite sick leave, because if it takes you two hours to realise you can't read the screen you are clearly not accomplishing anything by going to work, you are probably either physically ill or severely burned out, and you will therefore need at least some relaxation time and possibly some medication or therapy to switch your brain back on.
goblin89 12y ago
> But something tells me you aren’t a unit tester. Hah! I often think along the same lines when reading code. (Except exhibit 3—but I'm starting to arrive at it.) I also see my own past mistakes there.
ggchappell 12y ago
This is an interesting idea. I think it would benefit from one or two examples of good code.
AnotherBlueMoon 12y ago
> And whilst I can work out that that 20 line block in the > middle is used to decide which users we need to send emails to Yeah but no, you can't do that with text color.
_cipher_ 12y ago
Nice way to pass a `rm -rf ~/' somewhere in there. :)

175 points by padolsey 12y ago

I've seen this type of analysis referred to as looking at the "shape of the code" and I personally like that metaphor. If you see any "clumps" or "spiky edges", that might be a sign to investigate further and see if the code is trying to tell you where the problems are. I find this technique useful when you are asked to review something but you don't have full context of the project/language/library - I could point out potential issues with e.g. a Go program, even though I've never written a single line of Go myself.

michaelfeathers 12y ago

Nice article but I think there's something missing. Some things are uniformly bad like multi-screen methods or methods with six levels of nested conditionals, but the shape of code isn't really uniform. You can't look at the class or method level and play the three bears game (this porridge is too hot, too cold, just right). In codebases, the ratio of big to small matters. It's fine to have some of those classes that seem too small. They are a natural consequence of good structuring. As well, it's okay to have a few large classes also, ones that you know could be broken down but not convincingly. The thing that's awful is that if you say this sort of thing people want to believe that it's license to allow classes and methods to grow without bound, but it isn't. If you aim for understandability, you'll have code with sizes that fall into a distribution, usually power-law-ish https://researchspace.auckland.ac.nz/bitstream/handle/2292/16834/Understanding%20the%20shape%20of%20Java%20software.pdf?sequence=2 https://researchspace.auckland.ac.nz/bitstream/handle/2292/1...

kylebrown 12y ago

> And whilst I can work out that that 20 line block in the middle is used to decide which users we need to send emails to if you give me an afternoon and some strong coffee, I would humbly suggest that you stick it inside a def users_to_send_emails_to so that I don’t have to. Love this part, seconded.

chrisweekly 12y ago

Thanks for sharing a good post. I think of these code "shapes" as decent examples of the more generic metaphor of code "smells". But sometimes you need more context / other senses to determine whether or not the choices at hand were pragmatic. For larger systems, tradeoffs involving local complexity vs global simplicity can't be judged in a vacuum.

ashmud 12y ago

When I read "without your eyes", I was thinking more of code analysis tools (of the "style" variant).

jonaldomo 12y ago

I enjoyed this post. I really thought this was going to be about using visual regression for code reviews. I do think you could probably automate some of this though which would make for an interesting framework!

saganus 12y ago

I'm not sure if I understood the article fully, so I would appreciate if someone could give me an explanation. From what I understood, and by reading the comments, it seems like there are certain patterns in code that could be somewhat-universally flawed? Like seeing a lot of indentation without meaningful code in between? Or am I getting this wrong? Is it really possible to establish a "score" of sorts for how the code looks without actually looking at what it does? If it is, then it would seem like an interesting idea to build a code "shape-analyzer" that would give you the same type of analysis. Or is this too hard to describe in terms of patterns that we can see but can't explain? I think I need my morning coffee... am I making any sense?

Cthulhu_ 12y ago
You are, but maybe the article isn't :p. I can identify with the post, but maybe a bit differently; when I read code, especially in a language I'm familiar with, I see patterns instead of individual lines of code. So when a piece of code breaks the patterns, or breaks a rhythm, or looks different from the other code, my spider sense tingles and I frown upon the code. There could be nothing wrong with it, but it looks different so I end up refactoring it until it fits the rest of the codebase and I understand it.
- saganus 12y ago
  These patterns you speak of, are they on your own code or could you do the same with some one else's code? I ask this because I think I could do it with my own, but not sure if I could with another one's. Or maybe to the odebase of a particular product instead?
goblin89 12y ago
OP presents a few common anti-patterns in a humorous way. The shapes are how these anti-patterns might look in Ruby code, but anti-patterns themselves are language agnostic. Shapes are really just for illustration, but they also emphasize the irony: “you’re part of a preternaturally enlightened dev team” yet you review your coworkers' code without even reading. Perhaps there's something to be read between the lines. :)

Monkeyget 12y ago

You can do this more efficiently and for the entire code base at once by using code visualization tools: CodeCity [1]. A city represents a code base. Each class is represented as a building. The aspect of a building gives information on the corresponding class. For example: the color is the number of lines, the height is the number of methods and the width is the number of attributes. System Complexity View and Class Blueprint [2]. Like CodeCity the color and shape represent various attributes. An added benefit compared to the codecity is that you can also show links between the code elements. For example inheritance or invocation. NDepend [3] for .NET also has many interesting code diagrams. [1] http://www.inf.usi.ch/phd/wettel/codecity.html http://www.inf.usi.ch/phd/wettel/codecity.html [2] http://www.moosetechnology.org/docs/polymetricviews http://www.moosetechnology.org/docs/polymetricviews [3] http://www.ndepend.com/Features.aspx#Diagrams http://www.ndepend.com/Features.aspx#Diagrams

saganus 12y ago
That CodeCity idea is very very interesting.
- jagtesh 12y ago
  Apparently, it's not just an idea: http://www.inf.usi.ch/phd/wettel/codecity.html http://www.inf.usi.ch/phd/wettel/codecity.html
  - saganus 12y ago
    True that. I meant not as an unrealized idea but as in generally, a good idea. I did check out the software but unfortunately it's not for commercial use. Still, neat way of analyzing code.
anigbrowl 12y ago
I don't understand why tools like this aren't built into every editor. Syntax highlighting and code folding are great, but those are ~30 year old innovations; adopting the 'blurry text' approach of the OP (where you can't read specific things), consider the general lack of visualization tools for utterly basic things like loops and branches. How come we don't automatically generate function flowcharts, for example? I use a lot of flow-based programming tools for DSP and the more time that goes by the more perverse the text fetish of many programmers appears, like a statistician that hates graphs and insists everything has to be presented in tables.

CaptainQuirk 12y ago

Excellent topic. It resembles something I've heard graphics designer do when they try to appreciate the information hierarchy or the contrast on a creation, without being disturbed by the meaning of words of symbolic value of graphical elements. Although, I disagree about comments. I mostly overcomment code, because sometimes, a project lacks a proper specification and it's sometimes the only place where to explain what you actually do.

nikisweeting 12y ago

Great article. I especially loved "to find out what the foo is going on". Does this mean I may gain some higher insight into my code by blurring my vision? I don't have any glasses to forget...

ckdarby 12y ago

After reading this I must ask you to please take the time to have a redundancy plan in place for your glasses; I recommend a secondary pair offsite at your work in order to prevent outages of lost/forgotten glasses.

hyperliner 12y ago

OP forgot that the main ingredient for a code review is TRUST. He should have simply told the dev that he forgot his glasses. Then asked if the dev would help a little. And yes, ckdarby's comment of getting a redundancy plan in place for your glasses is spot on.

emackn 12y ago

Similar to the Sandi Metz talk, All the Little Things, from rails conf. She talks about the "squint test" around the 5 minute mark. http://www.confreaks.com/videos/3358-railsconf-all-the-little-things http://www.confreaks.com/videos/3358-railsconf-all-the-littl...

angersock 12y ago
Wonderful talk. Really funny is the bit about "Yeah, when I get called in, it's not to be like, hey, look at this wonderful code!"
- mjcohen 12y ago
  I agree. I really enjoyed her talk. It filled my head with a number of interesting ideas, some of which I might even use. "Small is beautiful."
girvo 12y ago
That was an amazing talk that really made me understand why SOLID and small objects are worth the initial hesitancy. Thanks so much for linking it, I think the people at work are going to love it.

DougWebb 12y ago

The article seems to be saying "I can't see anything you've written but I'm going to review it anyway: no matter what you've done, I don't like it." Maybe it's just the way the article is organized, or maybe I gave up reading the "I don't like this" examples before I got to one that said "This is good".

fryguy 12y ago
The not being able to see anything is just being clever. The article is really about bad designs that are identifiable by their structure alone. It's obviously going to primarily talk about "bad" ones because those are the interesting points.

JasonFruit 12y ago

It seems like Jeff Atwood wrote something similar to this several years ago, where he recommended pasting your code into Microsoft Word and dropping the font size to illegibility so you could see the patterns of indentation without being distracted by reading the code. I remember it being about equally valuable, without the undertone of snark I get from this article.

tjr 12y ago

Exhibit 5 Clarificatory comments, thumbs up! Code that requires an accompanying multi-chapter essay to understand, thumbs down. Looks like about 150 words of comments there, not a multi-chapter essay. That aside, I don't know what this code is doing, but working on sundry avionics software I would find that approximate amount of commentary very helpful in some situations. I would be remiss to downplay the value of this commentary without reading the actual words.

yellowapple 12y ago
Same here. And even if that amount of commenting isn't necessary, I'll take excessive commenting over insufficient commenting any day.
- m_mueller 12y ago
  Depends on the comments. IMO comments together with the code should still follow the DRY principle - since code states exactly what it does, the comments should only state why it does what it does - except if we're talking about comments intended to be exposed to the documentation. Otherwise comments become outdated very quickly, ensuing confusion further down the line.
  - wiredfool 12y ago
    The most valuable comments that I've found are documentation as to exactly why the code is doing something that looks strange or wrong, on purpose. It keeps maintainers from going in and breaking stuff later. Yes tests. But people can 'fix' tests at the same time too.
Argorak 12y ago
Except one, all those comments are on top of methods and the class. This can quite possibly be interface documentation. For a public API, this wouldn't be a lot of docs.
patrickg 12y ago
Agree. I have some complex code that I revsit once in a month, and I cannot afford thinking though it again and again. Therefore my comments are getting larger and larger and more chapter like with many images inbetween. (I use locco for that, see http://speedata.github.io/luaqrcode/docs/qrencode.html http://speedata.github.io/luaqrcode/docs/qrencode.html for a different but similar example.)

jacalata 12y ago

You’re part of a preternaturally enlightened dev team, and have set aside an entire day for nothing but code review. However, after the first 2 hours, you realise that you have forgotten your glasses and have just been staring at colourful blurs all morning. What do you do? You go talk to your boss and tell them you are taking indefinite sick leave, because if it takes you two hours to realise you can't read the screen you are clearly not accomplishing anything by going to work, you are probably either physically ill or severely burned out, and you will therefore need at least some relaxation time and possibly some medication or therapy to switch your brain back on.

goblin89 12y ago

> But something tells me you aren’t a unit tester. Hah! I often think along the same lines when reading code. (Except exhibit 3—but I'm starting to arrive at it.) I also see my own past mistakes there.

ggchappell 12y ago

This is an interesting idea. I think it would benefit from one or two examples of good code.

AnotherBlueMoon 12y ago

> And whilst I can work out that that 20 line block in the > middle is used to decide which users we need to send emails to Yeah but no, you can't do that with text color.

_cipher_ 12y ago

Nice way to pass a `rm -rf ~/' somewhere in there. :)