17.1 Code Reviews

I l @ ve RuBoard

A code review is the process by which the programmer shows her code to her peers and they review it. Code reviews are the most effective way of making sure your code has a minimum number of bugs . Code reviews not only give you better code but also give you better programmers.

Producing an effective code review is an art. It requires good people-skills and management support. It takes time to get a good system in place.

17.1.1 Planning the Review

The first thing to consider when planning a code review is who will be on the team doing the review. Ideally you should try to have three to five people there in addition to the person who wrote the code. Fewer than three and you don't have enough people to do an effective review. More than five and you can't have an effective meeting. (In large meetings, people don't talk, they give speeches.)

At least one senior software engineer should be part of the group . He's there to make sure that the code being reviewed conforms to the overall design of the project it's written for. He also has enough experience so that he probably can spot more problems than the less experienced coders.

Ideally, you don't want management there. That's because whenever a non-technical boss shows up, you spend more time explaining things to her than reviewing code. If a manager decides she must be there, make sure she knows that her role is strictly as an observer.

One problem with code reviews is getting the team to actually attend the meetings. Bribery is one solution to this problem. Free drinks, cookies, bagels, or other eats can be used to help attendance. Lunch should also be considered .

17.1.2 The Review Meeting

All right: you've chosen the team, you've got a conference room, and somehow managed to get everyone into the meeting. What now?

First, you need to designate someone to take notes. This person's job is to capture all the important information generated in the meeting. This person should not be the writer of the code. The author should keep his own notes. That way, with two sets of notes, you can be sure that any changes recommended by the committee are performed.

The meeting starts with the author of the code explaining his work. How he does this is largely irrelevant. He can use the top down method, bottom up, or sideways with a left twist. As long as the other people in the room understand what he's done, he can explain his code any way he wants to.

The other people in the room should feel free to break into the conversation at any time with questions or suggestions, or when they notice a problem.

Comments should be constructive. Everything said should help make the code better. Comments such as "I think you need to check for an error return from that system call" fall into this category.

Comments like "I could have done that in half as much code" are not appropriate. This is not a competition. This is also not a design review, so don't criticize the design (unless it's flawed and will absolutely not work.)

It is important to keep the meeting on track. The purpose of the review is to make sure the code works. The meeting should be focused only on the code in front of you. Topics like new programming techniques, the latest Microsoft product, and what movie you saw last night do not belong here. They should wait until your regular gossip sessions when you and the rest of the gang hang around the copy machine to discuss the latest rumors.

Code reviews should last about one to two hours, three at the most. Any longer than that and the committee will start to skim code and skip steps just to get out of the meeting. Programmers are like a can of soda: after about three hours, they go flat and lifeless. If you need more time, break it into multiple reviews, preferably with different people doing the review each time.

If you do a review right, the result will be that you've caught a number of errors. But what's important is that those errors did not make it into the final product. Fixing something in the testing stage or, worse , after the product has shipped, is extremely costly.

There's one key advantage of code reviews that we haven't discussed yet: you not only get better programs, you get better programmers. When a mistake is caught at review time and pointed out to the programmer, she is probably not going to make that mistake again. The result is that the next program she generates will contain less errors.

One example of this occurred when I wrote the book Practical C Programming . I had a bad habit of using the word "that" in places where I shouldn't. The copy editor reviewed the book and crossed out about three to five "that"s per page. I got the job of editing the files and removing the approximately 2,000 extraneous uses of the word. As a result, O'Reilly and I produced a better book. But another result was that in all my subsequent books, I didn't make that mistake again.

17.1.3 Why People Don't Do Code Reviews

One of the main reasons that people don't do code reviews is that they are working on a tight schedule, and management has decided there isn't enough time to do them. This is a false economy. After all, is the goal to get code out on time or to get working code out on time? (If you eliminate the requirement that the code must work, you can cut down on development time tremendously.)

Code that has not been reviewed will be buggy . Hopefully these bugs are found during the testing phase where they are merely expensive to fix. Sometimes they make it into a release product where they are very expensive to fix. One of the cheapest times to fix bugs is during the coding phase, and code reviews are an excellent way to see that bugs are eliminated early. (As someone once put it, "Why is there never enough time to do things right, but always enough time to do them over?")

17.1.4 Metrics

Metrics are an important part of the code review process. They show how effective your review process is. With proper metrics, you can demonstrate to both the programmers and management that code reviews are effective and are saving the company money.

The metrics that should be collected for the code reviews are:

• The number of lines reviewed

• The time spent reviewing the code

• The number of defects found

• The defect density (defects per line of code) ”computed

• The review rate (# lines reviewed per hour ) ”computed

Most review processes show a remarkable ability to find defects before the code enters the testing process. (There it costs 10-500 times as much to find defects.) Also, as you progress, you'll discover that the defect density goes down. This is because the review process not only makes for better code, but better programmers.

I l @ ve RuBoard