Talk About Quality

Tom Harris

Code review is code use

with 5 comments

Wordle: Code Quality Jesse Gibbs at Atlassian sent me to the following post from Scott Bilas at the game software company Loose Cannon Studios. In Peer Code Reviews: Good Commenting Practices, Bilas says code reviewers should seek architectural issues, and adherence to good software development practices and coding standards. And that they should look for mentoring opportunities. At the same time, he lets them off from other responsibilities, saying “Reviewers aren’t expected to catch everything,” and, “Reviewers aren’t expected to catch deep or systemic design problems.”

It’s a pretty good tutorial on the current best practice of code review. So why does it feel like something big is missing?

Upon reflection, it turns out that, unlike, say, editing and commenting on a book, code review is not really a reviewer-author relationship at all. An editor may make a lot of changes, but those changes end with publication. Code, on the other hand, will be fixed, extended, and refactored by future developers. Each future, or “next” developer will need exactly two things from the code in order to do his or her job: readability and understandability. Code is readable if it has the right balance of abstraction vs. detail at each level of the code. Code is understandable if the reviewer can, without help from the author, see what the code does, and why. If code is clear, that is, readable and understandable, modifications will be easy and error-free.

So the question “what are the responsibilities of a code reviewer” turns out to be a trick question. Like this one, from driving class:

Question: In the following traffic diagram (imagine any diagram you want), who has the right-of-way?

Answer: Nobody has the right-of-way. Right-of-way can never be possessed, only given.

The code reviewer does not have any review responsibility. The code author has all the responsibility to write clear code, and provide it to the code reviewer. We should stop calling that person a “code reviewer”; instead, say “code user”.

Code review becomes a simple and easily explained two-step process. First the “code user” reads the code and notes down what s/he doesn’t understand how to use, and second, s/he meets with the code author to present what was not clear. In that manner, the “code user” successfully stands in for the “next developer”, and gives the code author an early chance to make things better. As a bonus, all the things a traditional code reviewer was supposed to check, or at least the important things, will be checked. Mentoring will happen. All this when we realize that code review is code use.

About these ads

Written by Tom Harris

July 28, 2009 at 1:06 am

5 Responses

Subscribe to comments with RSS.

  1. I like the idea of ‘code users’. In practice, I suspect that many of the folks reviewing code are going to reuse some of the methods/classes they are looking at in the future, and possibly maintain it as team members and responsibilities change.

    At Atlassian, some of our teams submit every changeset for code review to the rest of the team. Other members of the team aren’t required to review every commit per se, but in practice most team members look at the commit to some level of depth, since we tend to rotate responsibilities within teams quite a bit to keep them ‘cross-trained’.

    Jesse Gibbs

    July 28, 2009 at 6:36 am

    • Note that to justify this idea of code reviewer as “code user”, s/he does not have to be someone who will actually maintain that piece of code in the future. Rather, s/he stands in for the real “next developer” who will. Often the “next developer” is precisely not the code reviewer, but the code author, the following week or month. But that “standing in for” attitude is what makes the method work. Although I hadn’t intended it, we might see it as a novel application of the Agile principle of bringing the customer close to the developer. In this case, the customer of the code is the “next developer”. If you don’t have such a customer, invent one — the code reviewer as “code user”.

      Tom Harris

      July 28, 2009 at 8:58 am

  2. I like that! Just a few comments:

    1. The same can be applied to any other deliverable: The design, the requirements…

    2. The same should be applied to the entire product: does your testing actually verifies the quality of the product from the user’s perspective?

    Lidor

    Lidor Wyssocky

    August 5, 2009 at 10:31 am

  3. […] Revisões são sobre a compreensão do código, certificando-se de que ele funciona direito, não de criticá-lo. […]

  4. […] Reviews are about understanding the code and making sure that it works right, not criticizing it. […]


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

Follow

Get every new post delivered to your Inbox.