Throwing Darts at Code

I’ve been thinking about code reviews a lot lately.

My mom has often worked as an editor, and she once told me of the most difficult thing about being an editor (I paraphrase):

You have to let the writer keep his or her style. The challenge is to improve the writing without making it seem like you wrote it.

Working as a technical writer and as an English teacher, I thought about these things a lot. I would often think, “that is not a sentence that I would have written, but it is clear. It works.” Sometimes I would read a sentence and think, “I don’t know what that meant; that sentence doesn’t work.” I would change the sentence in the second situation but not the first.

Code reviews, on the other hand, make me think of the warning my dad gave me when I wanted to take a creative writing class in college:

All you’re going to do in a creative writing class is put your story up in front of the class once a week and let people throw darts at it.

I think what my dad meant is that my classmates were not going to be good editors; they were going to try to make the story, or essay, or whatever, look like they wrote it, rather than trying to improve it without imposing on it. Not that they would do this on purpose; it’s just that when we ask ourselves about a creation, the critiques we come up with are about things that surprise us. That is, they’re about things that the creator did that we wouldn’t have done.

Many code reviews I’ve been a part of haven’t worked very well. We end up rehashing composition vs. inheritance, or complaining about some design pattern. People say they feel ill because there are so many if statements. (In fairness, there were a lot of if statements on the projector that day, and everyone agreed that the code needed refactoring, but it still seems a needless exaggeration.)

Perhaps part of what I object to is tone. Let’s say I put a really confusing unit test on the board. The variables have weird names, and it’s not clear from the assertion what was happening. There are two approaches to pointing this out:

Version 1:

I am physically ill from the number of bad variable names. What does this even mean?

Version 2:

I didn’t understand that test the first time I read it. Do you think we could change some variable names?

The information content in these two criticisms is binary identical. The person is pointing out that a test is hard to read, and that maybe confusing names are the problem. But the second approach avoids any invective. I wish we would prefer the second approach. Call me oversensitive, but people work hard on their code, and no one wants to be told that their baby’s ugly.

Another part of what I object to is criticisms that boil down to, “I wouldn’t have done it that way, so you’re wrong.” Sometimes these are couched in weirdly academic objections: “I’ve never liked the Builder Pattern.” Ok, that’s great. You don’t like this design pattern because it called you names as a child? Is this code hard to read or incorrect? Sometimes these might be real objections, but they way they are often phrased, they seem silly.

On the other hand, we do want a codebase to become coherent. If half the team uses _ at the beginning of instance variable names and half the team doesn’t, it will just be a mess.

Ideally, I think that these stylistic considerations should, as far as possible, be offloaded onto the linter. When I run Rubocop on my code, and it says, “don’t start method names with get_,” then that’s fine. I go rename my method before I even push the commit, and we don’t waste anyone’s time arguing about that name.

I think I now have a few things I wish happened at code reviews:

  1. That people expressed differences of opinion without invective. “I had trouble reading this,” is much better than “this is unreadable.”
  2. That objections be made more on the basis of technical correctness and readability, rather than merely on personal opinion.
  3. That rote things, like tabs vs spaces and whether methods can start with get_, be handled by an automated tool, so that we don’t waste time talking about them.

Anyway, these are just my thoughts on code reviews. Is there something you wished was different about code reviews? Please leave a comment below.

Till next time, happy learning!

-Will

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s