Talk About Quality

Tom Harris

Code Review for People

with 2 comments

Sometimes I get the feeling that when people consider and talk about code review, they’re talking about different things but they don’t know it. I see code review everywhere during coding.

Try this: put aside the image of code review as a meeting, or even code review as an independent activity. Limit your description of code review to: someone else is reading your code, you’re discussing it together, and there’s effective writing as a result. If you see those three things happening, you’re seeing code review.

Debugging

The great hope was that code review would be solely a preventive activity: do code review before execution, and there won’t be anything to debug. That’s not going to happen. Too much code is delivered under pressure, without code review. But all is not lost. Just review the code when debugging. Look again at the three steps — how does code review, as defined above, change debugging? First, don’t debug alone. Whatever you may think of pair programming for or against, pair debugging is a must. Especially if you wrote the code, having someone else look for the problem, or better, restate the problem, is essential. Second — discussion. Debugging that rushes to a fix often produces the wrong fix. And finally, effective writing. Sure there’s the writing which is fixing the code. But what about adding comments? And perhaps more important and effective, especially if it’s a quick fix: submit a new defect or change request to your tracking system for what really needs changing to reduce the chance of this failure recurring.

Static Analysis

Tools! If I have tools that can find problems, I won’t have to review code. Yes, you will. And you should want to. Static Analysis tools are great for finding simple errors, and potential defects. For narrowing the field of focus. But then, to find out what’s really wrong … code review. Additional benefit of the tool — since it has no feelings, it can be two of you against the reviewer: read the code with a partner, discuss why the tool thinks there’s a problem and whether you agree, and again, effective writing. Not just the fix, but why that fix, and also perhaps an adjustment to the tool so it will do better next time. High-end static analysis systems let you document that right inside the tool.

Testing

Testing, and its partner, code coverage, are often seen as the opposite of code review. Whatever I couldn’t catch by code review (and static analysis) is left for exercising the code under controlled conditions, causing failures, and fixing them. Actually code review is the most important part of developer testing. Not only reviewing the code under test in order to design good white-box tests. But also reviewing the code based on the test results and coverage results. Why does the software intermittently fail this test? Why is it so difficult to cover all the branches in this function? Again, read together, discuss, write. Here the effective writing is likely to be refactoring the code. If someone wants code review “documentation” — give them a diff and a one-sentence explanation of what you did and why.

Code Review is All-Purpose

There are many more activities in the developer’s daily life. Detailed design, branching and merging, delivery — all can benefit from in-line code review with this three-step model of reading, discussion, and effective results-writing. And by acknowledging this model, we see that code review, or almost code review, is happening all the time. Just have to complete the steps to get code review that real people will do.

Written by Tom Harris

February 11, 2010 at 11:06 am

Posted in Code Review

2 Responses

Subscribe to comments with RSS.

  1. Disclaimer: I work for Atlassian, a company that makes code review tools.

    Great article! The idea that ‘code review’ has to involve meetings is unfortunate, because it detracts from the value of the practice (early bug detection, pre-emptive refactoring, etc.

    All of these practices (debugging, static analysis ‘result sifting’, and test design) can be done in real-time (pair programming, IM).

    Static analysis results seem well suited to using an asynchronous tool if you can’t meet together. In a previous life working for a static analysis tool vendor, I often viewd heated debates at a customer site regarding whether a particular bug was valid or not. These discussions were really valuable in increasing the shared knowledge of the team, but were limited to the folks in the room.

    Since a lot of the bugs found by a static analysis tool are “important but not urgent” (e.g. you shouldn’t release before fixing them but they aren’t breaking the build or failing a test), it seems like discussion about these results could be done by a distributed team using an asynchronous, threaded discussion.

    I would be curious to know what other sorts of activities could be lumped under code review, and ideas about whether they must be done in real-time or if they can be done asynchronously.

    Jesse Gibbs - Atlassian

    February 12, 2010 at 8:49 pm

  2. Great post! We’ve added it to the list we maintain of good content about code review:
    http://smartbear.com/code-review-articles.php

    Lisa Wells

    February 13, 2010 at 2:10 am


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