Code review without your glasses | Hacker News Reader

Code review without your glasses

robertheaton.com

175 pointspadolsey42 comments

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

[2] http://www.moosetechnology.org/docs/polymetricviews

[3] http://www.ndepend.com/Features.aspx#Diagrams

That CodeCity idea is very very interesting.

Apparently, it's not just an idea: http://www.inf.usi.ch/phd/wettel/codecity.html

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.

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.

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

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.

You had me at 'syntax'.

The fact that we're still typing crap into windows and calling advances along this one-dimensional path 'innovation' is pain.

Intentional[1] had a lot of promise but was too slow off the mark and now feels more like BPML, which has too much XML hiding under the covers to be good. As it stands, making solution development easier for business users has not had a broad impact on the overall productivity of developers.

'Blub' assumes existence on a continuum of language. It's a limiting concept in itself, as many of us have a different internal representation of thought.

It's sad to read about "python stuff for swift" or "haskell stuff for scala" when the only advantage of those exercises is as homework to understand the concepts.

[1] http://www.intentionalsoftware.com/

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.

Same here. And even if that amount of commenting isn't necessary, I'll take excessive commenting over insufficient commenting any day.

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.

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.

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.

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 for a different but similar example.)

I agree with you, especially when you're writing low-level code which must interface with hardware or implement some protocol. However please allow me to play devil's advocate for just a moment.

First, incorrect documentation is harmful. So much so that in many circumstances I'd rather have no documentation than incorrect documentation. If all of your code has such a high comment-to-logic ratio you're either writing some really subtle (perhaps math-heavy) stuff, or you're introducing a bunch of potential for documentation issues.

Second, readability counts. If you feel like your code isn't understandable without a large number of comments, you've probably not written it to be readable. Sometimes this is very difficult to do (yes, I'm looking at you hot-path C++ code!) but I think that past a certain point effort can be better spent making the code easier to understand than on verbose documentation.

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

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?

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.

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?

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. :)

Ah. Much more clear now.

Thanks.

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.

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

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

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

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.

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.

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.

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.

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.

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

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.

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

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!

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.

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

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.

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

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

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

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

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

Reply on news.ycombinator.com