Two Against the Code

One of the big human challenges to code review is ego. For all the talk about “egoless programming”, ego still means “self” and when a person writes code, or owns it for maintenance, s/he identifies with the code. That kind of “ego” or “self” thinking is what lets the code author work through problems by saying, “let’s see, when I receive this signal, I enter this loop and I calculate that value.” So how can code author and code reviewer(s) go into a code review, whether it be a meeting or online, and yet avoid criticism and defensiveness?

Here’s an image that can help: when code review participants enter the review, imagine everyone sitting on the same side of the table, with only the code on the other side. The code author is no longer the author or owner — just another reviewer. The two or more people in the room, or in the online review, form a team together to find as many things as possible in the code to improve. Now nobody needs to defend the code, because everyone is a reviewer. Still, though, if criticism is in the air, the real code author will feel it.

The desired result of a code review is a list of important things to improve. Comments which the code author can take back to the code and say, “I understand what I need to do to make this code better.” Take a concept from the topic of assertive communication: The “I” message. Best explained by an example:

Let’s say I’m a busy, pressured group leader who has to lead meetings, and someone is always late for the group meetings — much later than everyone else . I decide to speak to that person. What can I say to him that will cause him to feel what I feel, and based on that, to join me in solving the problem? Not to feel criticized, and to become defensive. An “I” message would be:

“I’m under a lot of pressure when leading our weekly status meetings, and when I see you come in 20 minutes late to the meeting, it makes me more tense, and makes it hard for me to keep the meeting on track.”

The idea is that, while I have mentioned the person’s lateness, I have described only what I see, and described a problem that I have. All the focus is on me and my problem, which the group member may realize is his problem too, since he may benefit from a calmer group leader and a more effective meeting. Hopefully, he is drawn into solving my, or our, problem.

Similarly with code review. Instead of a critical comment like, “this code is very confusing,” say, “when I read through this function — both the comments and the code — I get to the end and I still have no idea what it’s really supposed to do. If I had a change request on this code, I would not be able to confidently make the change.” Given that the code author probably does not have the same problem, this comment draws the author into saying, “I understand what the function is supposed to do, but apparently you don’t from reading the code. I’ll try to improve the comments or clarify the code so that you understand what I understand.”

Try this method in your code reviews to get everyone working together against the code, to improve the code.

Advertisement

Code Review for People

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.

What’s the difference?

What kind of tool is required for code review? What are we trying to accomplish? How much of the code must be reviewed at a time, and why? These are some of the questions that we get caught up in, that prevent us from deploying effective code review in a development organization. One way to cut through the arguments and move forward is to realize that there are different types of code review, with different goals. Here are some parameters of two major, but different types:

Code Review type 1

Key question: Does this change accomplish what it was supposed to, while doing no damage?
Desired code author response to comments: Revise changes
Under review: changed files with diff
Tool: diff comparer
Required feature as a code review tool: right-click submit comment
Nice-to-have features (but could be in a separate app): right-click definitions

Code Review type 2

Key Question: Is this code designed well, so that it is easy to fix and extend?
Desired code author response to comments: Refactoring / renaming
Under review: any subset of entire codebase, starting from a given area of the code
Tool: source code browser
Required feature as a code review tool: right-click submit comment
Nice-to-have features (but could be in a separate app): code editing

Code review is code use

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.

The Only Valid Measure of Code Quality

It’s Thom Holwerda — keeping things simple for us:

Simple Code Quality Metric

Here’s his site — with a search for “code quality” (warning: if you can’t ignore “Evony” web game ad graphics, stay away).

Really simple code review tool

I know what I want: highlight-and-comment capability in a text editor. And since plain text really shouldn’t be marked up, I’d want it to save the comments in a separate text file, with line numbers of what was commented on, and the comments’ text. I’m not the first one looking for this capability: “climbanymountain” asked about it on Joel on Software back in 2006: “win32 text editor for code review?”

I started thinking about writing a plugin for some popular text editor, such as Notepad++ or Notepad2 (the latter is my favorite for simple editing). But before the “build or buy” decision, I thought I’d ask around the office.

A lot of ideas, but here was the simplest. Light and off-the-shelf. Use a diff tool. 

The following procedure works fine, and “supports” any version control system. In my example, I’m using open-source WinMerge as my diff tool, IBM Rational ClearCase as my version control system, and Microsoft Windows Explorer as my tree browser. Say the source file to review is called “MyFile.c”, and the reviewer’s name is Firstname Lastname.

The parts in square brackets are optional: omit them if you don’t want to clutter up your version control system with code review records, but just want to use them to prepare for sitting with code author.

1. Browse to the source code file I want to review; [check it out and] open it.
2. Immediately Save As MyFile_flastname_review.c in the same directory. Close the source code file.
3. Select the source code file and the review file (they are next to each other in alphabetical order), right-click and open WinMerge on them.
4. Go to the pane of the review file and type review comments anywhere I want. Save regularly with CTRL-S. (Optionally, for better syntax highlighting, can precede comments with comment character such as “//”. But don’t have to.)
5. Generate a diff (patch) file, call it the same name as the review file, but add .diff on the end. (In WinMerge, that’s on the menu with Tools > Generate a patch.)
6. [Check in all 3 files (adding 2 new ones to source control) — check-in activity per defect tracking system, and comment with one-line summary of the review.]

From this point, code review comments are easily viewable side-by-side with the code by viewing in WinMerge or similar viewer.

Also, the diff file clearly identifies the line number commented on, and the comment text. I figure I could write an AWK script to turn that into CSV, for tabular display and tracking in Microsoft Excel, if I wanted to.

Essentials of Code Review

Why Code Review?

We write code for two audiences. One audience is the hardware: the compiler, and the platform where the executable runs so that the user can operate it. The other audience, no less important, is the “next developer”: someone else (or even yourself a few months later) who has to modify the code to fix it or add new features.

The hardware’s reaction to the code is clear from testing. But the only way to find out if the code is easily maintainable is to have someone else—a knowledgeable developer—read the code. And the earlier, and more often, the better, since errors can be caught sooner and cheaper as well.

Additional benefits of code review: improving the code author’s coding (footnote 1) through the experience of guided correction, and finding and preventing systematic errors.

Code Reviewer and Code Author are Partners

The code reviewer must know the programming language well, be familiar with the technology (footnote 2) that the code implements, and be patient and effective in giving constructive criticism. The code author is the current maintainer of the code. Both must see themselves as working together to improve the code.

Review Comments, Reactions, Tracking to Closure

The code reviewer reads the code and makes comments. The code author responds by changing the code to improve it according to the comments s/he accepts. All comments must be recorded, electronically, both for tracking to closure, and for later review for systematic errors. If the author makes some changes during the review meeting, a “diff” of those changes may substitute for a list of those accepted comments.

First, Leave the Reviewer Alone to Read the Code

The reviewer first reads the code, alone, in a code browser that maps, and supports easy navigation of, the code’s structure. This method best simulates the “next developer’s” experience: the code author does not come packaged with the code—it must stand on its own.

Afterwards the reviewer meets with the author. If there are multiple reviewers, they may meet together with the author in a group review, but each reviewer must have first reviewed the code alone. A meeting should be in-person, but may be (less desirably) telephone or electronic, as long as it is interactive.

What Code Author and Code Reviewer Provide to each other

The author makes a labeled version of the codebase available to the reviewer. The requirements for the new, improved, or fixed code, and any required design, should also be known to the reviewer.

The reviewer reads the code for clarity = readability + 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. The reviewer may use detailed checklists of common errors, and use the coding standard as reference, but review for clarity will lead to all the others.

When to Review Code: At Minimum, Before Check-in to a Shared Work Area

Code may be reviewed at any time. Here are some different times, and the benefits of review at those times:

• As code is written: Find errors earliest, prevent repetition by improving the author’s performance
• Before static analysis: Avoid analysis time on code that doesn’t belong
• After static analysis: Avoid review time finding errors that a tool could find
• Before check-in: Maintain quality of the codebase
• After check-in: Allows time for thorough review
• Before delivery: Catch errors before testing group
• Before debugging: Narrow down debugging steps; possibly find error without debugging
• After field deployment: Understand problem areas reported from the field; make improvement recommendations

The team, group, or department lead may decide when to review code according to his or her desired benefit. However, if time constraints limit code review to just one of these stages, the required stage is before check-in to a shared work area, to maintain the quality of the codebase that others will learn from and use.

Which Code to Review?

All new or changed code should be reviewed as above, at least before check-in to shared codebase. More complex or historically problematic code—more thoroughly. Take care also to review the new or changed code’s “nearest neighbors” to make sure the code still works with existing code it interfaces with.

Footnotes

1. If there are multiple reviewers, the reviewers can also learn from each other’s review comments. But there are tradeoff costs of multiple-reviewer reviews: variation in quality of review, need for a strong moderator, and more time. Better for reviewers to learn from code review in the code author role, or in code walkthroughs specifically planned for cross-training.
2. There may be exceptions in special-purpose code reviews such as performance or security reviews, where the reviewer must then be knowledgeable in those areas, but not necessarily in the technology.

Too many reasons for code review

A co-worker forwarded me this article “5 Reasons for Software Developers to Do Code Reviews (Even If You Think They’re a Waste of Time)” which certainly sounded promising. Even when I don’t think code reviews are a waste of time. But as I read through it, it became clear that more is less. The article says too much, and detracts from its own message.

1. Developers know their code will be evaluated, so they work harder. “The most useful thing about a code review is the fact that the coder knows that someone is going to review the code,” says Oliver Cole, president of OC Systems and also lead for the open-source Eclipse Test and Performance Tools Platform project.

Work hard because you enjoy it. And of course your code will be evaluated, but not primarily by code review. Rather, the main user of your code is the “next developer”—possibly someone on your team, or even you yourself a few months later. That’s where the evaluation happens.

 2. It improves a developer’s own programming skills.
In your heart, you might not care that much about the success of this particular software project. But most programmers want to improve their personal skills, and that means learning from other people.

If you don’t care about the success of the project, code review won’t help.

3. It’s an opportunity for mentoring, so the programmers you work with get smarter (and thus, more fun to hang around with).” […] While the intention [to mentor individuals] is generally well meaning, it can often lead to individual discomfort and perceived or actual criticism. In these cases, the greatest opportunity for mentoring usually exists in the context of small collaborative teams working together to realize goals and not in a code review.”

Criticism is not bad, it is essential. It is not personal, but professional. And of course, the smaller the meetings (down to even just 2 people — reviewer and author), the better.

4. It creates consistency and a culture of quality across the project. […] Developers are quick to complain about being judged on the wrong metrics, but, says Gary Heusner, client partner at custom software developer Geneca, “We have to change the rules to allow for quality and efficient development to be valued over making milestones that are really yardsticks of process more than milestone of value delivered.” Code reviews are a big part of that.

Code reviews are simply part of good software development.  When management, together with the team, track value delivered, that is a big part of creating a culture of quality. Only when the environment is right can code reviews have a chance of being effective.

5. It encourages team bonding. “People think code review is just about finding bugs, but it brings people together, says Smartbear’s Jason Cohen. Often, he says, it can deliver far more than expected.

“Success stories happen very often when performing code reviews,” says Dave Katauskas, senior architect at Geneca. “But the best success story is the pattern that develops once a team has gelled. The longer you’re into a project, the better quality code is created. This is all due to the code review process and governance that occurred up stream in the beginning of the project.”

I had to read this one a few times. Right answer for wrong reasons. I will not credit code review where credit is not due. Even the writer with the “success story” realizes the true origin of the success is the gelled team.

But still, I had to click on the Jason Cohen link to see why code review “brings people together”. Go ahead—click below on “Lightweight Code Review Episode 5: Team Building for the Cold, Dark, and Alone”. But first, get ready to read it right: code review doesn’t create good teams. Rather, good teams benefit from code review. OK, now click.

Lightweight Code Review Episode 5: Team Building for the Cold, Dark, and Alone

Focusing Code Review

In an article Using Static Analysis to Find Bugs, in the September/October 2008 issue of “IEEE Software”, authors Nathaniel Ayewah et al conclude,

We need to develop procedures and best practices that make using static-analysis tools more effective than alternative uses of developer time, such as spending additional time performing manual code review or writing test cases.

In an otherwise fine article, just one key word–alternative–is misleading, promoting a common misconception. Static analysis tools are not an alternative to code review, nor can they replace it. Rather, static analysis tools are a guide to code review, helping the developer better use the time in “manual” code review. Of course, “manual” code review isn’t really “manual” — it’s not about the hands, but about the mind, understanding the code.

When using a static analysis tool, like the FindBugs of the article, or other types of tools, that measure and mark up the code for complexity or code coverage, the benefit is twofold. First, an automatic tool goes through a lot of code, and identifies which parts are most urgent to review. Second, the reports pose challenging questions for the code author or maintainer to answer by re-reading and explaining the code.

Where I work, on some projects at least, there is a formal policy for using static analysis tools. They are run automatically on all builds, and the results are tracked towards targets. But most important is the questions they generate: Why is this piece of code written this way? Is it what we meant? Why couldn’t the compiler, or lint tool, understand it without warning? Why does the code coverage tool think there are so many branches in these few lines of code? All these questions are generated by the tool, and mapped automatically to the code. But it still takes code review to answer the questions, and to adjust the code to make it better.

Read Before Running

Debugging: How to Start (commonly accepted method)

Search for “debugging” on the internet and most entries will tell you the first step is to reproduce the problem (properly called “the software failure”) and the second step is to create the simplest configuration that also reproduces the problem.

I agree that it is essential to reproduce the problem, because you need to know you’re working on the right problem.

Fun to Run, or “the thrill of the chase”

But programmers, myself included, like to run things—to let the computer do the work—so there’s a great temptation to reproduce the problem, change something, run the software, add some “debugging statements”, and run it again. And there goes an hour of valuable time.

Does readability fit in here somewhere?

If the code is written clearly, and by that I mean that it’s readable, with meaningful variable names and the proper levels of abstraction, then the second step might be to actually read through the code.

A short debugging story

Today, I showed myself both the wrong way, and the right way, to respond to a “bug report”.

Where I work, I’ve turned out to be the owner of a small set of text-parsing scripts, written in awk. We use them to pull out compiler warning messages from the long build logs, so that developers can see them and address them. These parsing scripts are short—half a page of code including comments—and are tested only with the few build logs that I’ve had time to try. Given limited time (it is not my main job to write parsing scripts!), I have to hope that all the build logs for a given compiler are pretty similar, so whatever pattern I’ve identified will be followed in other build logs. It isn’t always so.

A developer noticed (first by eye, confirmed with a simple “grep”) that there were more compiler warnings in his build log than in the parsed csv (comma-separated-value) output file. Not good. I set about debugging!

It took about 5 minutes to reproduce the problem. Really just gathering the sample input and bad output files, and setting up a copy of the relevant scripts. One run (takes about 2 seconds) showed that, indeed, some warnings were missing.

My next step, by the standard debugging technique, was to start printing all the partial results, run the script over and over again, and look to see where it was failing. Then some more time to adjust the failing script and retest.

Haven’t I seen this somewhere before?

After the first 5 minutes (reproducing the problem), I already had a pretty good idea of which script was failing. A sed one-liner that adds newlines after each compilation command line, and before the first warning message, so that the awk script would be able to separate the first warning message as a record and select it. What was odd was that when I opened that script (just one executable line—the rest is comments including change history), the last modification comment was about how I had fixed exactly the same problem before. Briefly I wondered about that, but rushed onward to more than half an hour of running parts of the script pipeline to prove to myself exactly what was failing and why.

I could have saved myself the time.

Debugging: How to Start (the code-reading method)

After reproducing the initial problem (again, just 5 minutes), and arriving at the suspect script with the worrisome comment, I should have stopped running the code, and started reading it. After all, if my last modification was to fix this same problem, then apparently that modification was either wrong, or insufficient.

It turned out, and this was visible by comparing the one-line script and the new build log on which it failed, that it was insufficient. The new build log had some extra spaces at the end of the compilation command line, and the sed one-liner, designed to identify such lines, was unprepared for the extra spaces.

It did take me another 15 minutes to verify that most new build logs had a random number of extra spaces, and thus to pick the right two-character adjustment to the regular expression. Regression testing took another half hour.

But I could have saved myself almost an hour in the middle of “debugging-by-running” if I had applied a few minutes of “debugging by reading the code”.

Even better would have been reading it out loud, or reading and explaining it to someone else.