17.1 Code Reviews
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.
|
