
Code review without your glasses - padolsey
http://robertheaton.com/2014/06/20/code-review-without-your-eyes/
======
Monkeyget
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)

~~~
anigbrowl
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.

~~~
nsajko
Probably because editors are editors and not code visualization tools? I hear
it is good practice to separate code and programs to minimal but purposeful
components. "Do one thing and do it well."

~~~
anigbrowl
Fine, every IDE then. I don't mean to be snippy but I remember similar
objections in the late 80s/early 90s to syntax highlighting on the grounds
that it was a crutch to lazy/incompetent programmers, and now I think you'd be
hard-pressed to find anyone who doesn't take it for granted.

------
tjr
_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
Same here. And even if that amount of commenting isn't necessary, I'll take
excessive commenting over insufficient commenting any day.

~~~
m_mueller
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
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.

------
michaelfeathers
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/1...](https://researchspace.auckland.ac.nz/bitstream/handle/2292/16834/Understanding%20the%20shape%20of%20Java%20software.pdf?sequence=2)

------
saganus
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_
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
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?

------
swanson
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.

------
emackn
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-
littl...](http://www.confreaks.com/videos/3358-railsconf-all-the-little-
things)

~~~
angersock
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
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."

------
jacalata
_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.

------
ckdarby
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.

------
CaptainQuirk
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.

------
chrisweekly
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.

------
DougWebb
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
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.

------
kylebrown
> _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.

------
jonaldomo
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!

------
JasonFruit
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.

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

------
hyperliner
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.

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

------
nikisweeting
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...

------
goblin89
> 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.

------
AnotherBlueMoon
> 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_
Nice way to pass a `rm -rf ~/' somewhere in there. :)

