Section 5.4. Code Reviews


5.4. Code Reviews

A code review is a special kind of inspection in which the team examines a sample of code and fixes any defects in it. In a code review, a defect is a block of code that does not properly implement its requirements, that does not function as the programmer intended, or that is not incorrect but could be improved (for example, it could be made more readable or its performance could be improved). In addition to helping teams find and fix bugs, code reviews are useful for both cross-training programmers on the code being reviewed and for helping junior developers learn new programming techniques.

5.4.1. Select the Code Sample

The first task in a code review is to select the sample of code to be inspected. It's impossible to review every line of code, so the programmers need to be selective about which portion of the code gets reviewed. Many teams have found that it takes about two hours to review 400 lines of code (in a high-level language such as Java), although this estimate differs dramatically from team to team and depends on the complexity of the code being reviewed. At that rate, there is no way a team could review all of the code for a software project. Nor would the team want toin any program, there is a good deal of uninteresting code that looks very similar to the code already developed in previous applications, which has a lower risk of containing as many defects.

The purpose of any inspection is to find and repair defects. Since a relatively small portion of the code will be reviewed, it's important to review the code that is most likely to have defects. This will generally be the most complex, tricky, or involved code.

There are a few useful rules of thumb that are helpful:

  • Is there is a portion of the software that only one person has the expertise to maintain? That may be a good candidate for review, for two reasons. First, because the rest of the team will learn how to maintain it; second, it's only ever been looked at by one person, so nobody else has yet had a chance to catch any defects in it.

  • Does the software implement a highly abstract or tricky algorithm? The more difficult the algorithm, the more likely it is that a programmer introduced errors in its implementation.

  • Is there an object, library, or API that is particularly difficult to work with? Working with a nonintuitive interface causes many programmers to make mistakes.

  • Was the code written by someone who is inexperienced or has not written that kind of code before? Does it employ a new programming technique? Is it written in an unfamiliar language? A programmer who is doing something for the first time is most likely to introduce errors.

  • Is there an area of the code that will be especially catastrophic if there are defects? A core tax accounting function is more important than the code that renders the splash screen. Select code that must not fail so that more people can look at itand will be able to maintain it if it does have problems.

It is important to select a sample of code that an inspection team can review in about two hours. The project manager should try to keep the meeting to two hours or less, to avoid "meeting fatigue."

5.4.2. Hold the Review Session

The team selection and preparation in a code review are similar to any other kind of inspection. An inspection team of 3 to 10 people must be selected. Each of these people must be technically capable of reading and understanding the code being reviewed. Before the meeting, the moderator distributes the code sample to each inspector, who does individual preparation exactly as in the inspection.

The main difference between a code review and any other kind of inspection is in the review session. While the code review session is similar to the inspection meeting (see "Page-by-page review" above), there are a few important differences.

In addition to the moderator, there is a code reader who reads the code aloud during the meeting. The code reader can also be one of the inspectors; the only requirement is that the reader must have enough technical expertise to understand the code. The purpose of the reader is simply to keep the team's place during the inspection; the team should have already read the code and identified defects during their preparation. Since code is usually organized in logical units or blocks, it is more useful for a reader to go through those, rather than having the moderator go through the document page by page.

The reader starts at the beginning of the code sample and announces the first block or logical unit. She does not literally read the commands in the code; she simply gives a brief description (about one sentence) of the purpose of that block. If anyone (including the reader) does not understand what the code does or disagrees with the interpretation, the author of the code explains what it is the code is supposed to accomplish. Sometimes the team can suggest a better, more self-explanatory way to accomplish the same thing; often it is simply a matter of explaining the purpose of the code to the person who raised the issue. If any inspectors found a defect in that block of code, the issue is raised and the team either comes up with a fix on the spot or tags it as an open issue for the programmer to fix later. The moderator then updates the inspection log, and the inspection continues until the reader completes the code sample being inspected.

Another important difference between code reviews and document inspections is that the code review is much more focused on detecting defects, and less on fixing them. This is because many defects in documents can be corrected with one or two sentences, or with a change in wording. Defects in the code can be much more involved, and there are often many ways that they could be fixed. The discussion of each defect is longer in a code review than it is during an inspection, and there are usually many open issues at the end of the code review.

In the code review, the moderator needs to be especially careful not to let the meeting turn into a problem-solving session. Programmers love to solve problems. It's easy for them to get caught up in a small detail and turn the meeting into an analysis of a minute problem that covers just a few lines of code. However, long discussions like this will prevent significant amounts of code from being reviewed. That's not to say that these discussions are not valuablethey just don't belong in the code review meeting. If a discussion looks like it will take more than three minutes or so, the moderator should stop the discussion and add it to the inspection log as an open issue. There should be few open issues, however, because most code defects should be straightforward to describe and document once they have been identified.

There are effective ways to modify the code during the review. Many inspectors have found that it is very helpful to refactor the code during the review. By applying refactorings on the spot, the team can make the code much more readable and identify additional defects. (See Chapter 7 for more information on refactoring.)

After the inspection meeting, the code author performs the rework and closes the open issues, and the moderator follows up with each of the inspectors and gains their approval. Instead of getting formal sign-off with physical signatures, it is usually sufficient to indicate the approval in the log comments when the changes are committed to the version control system (see Chapter 7 for more information on version control).

There are several additional benefits for the code review. One is that people learn how their teammates think about the code. A good way to encourage this is to switch off code readers in each review, so every team member gets a chance to be a reader. Reading code aloud and explaining it helps programmers think through problems. Every programmer should be able to explain his ideas well; discussing code during a code review is good practice for that.

Another benefit is that people who know that their code may be inspected tend to write more maintainable software. It's very common for programmers to not include comments or to write very terse, confusing code when they know that they are the only people who will ever read it. But if a programmer knows that someone else will be looking at it, he may put a lot of effort into making it readable. This can have enormous savings in maintenance efforts down the road.

5.4.3. Code Review Checklist

The following attributes should be verified during a code review :


Clarity

Is the code clear and easy to understand?

Did the programmer unnecessarily obfuscate any part of it?

Can the code be refactored to make it clearer?


Maintainability

Will other programmers be able to maintain this code?

Is it well commented and documented properly?


Accuracy

Does the code accomplish what it is meant to do?

If an algorithm is being implemented, is it implemented correctly?


Reliability and Robustness

Is the code fault-tolerant? Is it error-tolerant?

Will it handle abnormal conditions or malformed input?

Does it fail gracefully if it encounters an unexpected condition?


Security

Is the code vulnerable to unauthorized access, malicious use, or modification?


Scalability

Could the code be a bottleneck that prevents the system from growing to accommodate increased load, data, users, or input?


Reusability

Could this code be reused in other applications?

Can it be made more general?


Efficiency

Does the code make efficient use of memory, CPU cycles, bandwidth, or other system resources?

Can it be optimized?



Applied Software Project Management
Applied Software Project Management
ISBN: 0596009488
EAN: 2147483647
Year: 2003
Pages: 122

flylib.com © 2008-2017.
If you may any questions please contact us: flylib@qtcs.net