Tech:Reviewing Code
From Cyclopath
This page explains how to review others' code.
The goal of your review: ensure quality. Work with the programmer whose code you are reviewing to create a patch that will pass the project manager's review with no complaints. Specifically, you should:
- Identify bugs.
- Identify potential bugs, pitfalls, and design errors.
- Enforce Cyclopath coding standards and style.
Reviews should be finished promptly. Within 48 hours, you must either:
- Complete the review.
- Set a deadline for completing the review, and put this in the bug (and then complete the review within this self-selected deadline!).
Guidelines:
- If your initial review identifies no problems, you are probably not reviewing thoroughly enough.
- Be skeptical of your reviewee's code. It is better to identify a problem in error (false positive) than to fail to identify a problem in error (false negative).
Contents |
[edit] Who Reviews Whom
Say
reviewerrr cyclopath
to get a reviewer. (More details for Grouplens members) If you get someone who's not available (vacation etc.), do it again.
This deals out reviews round-robin. Undergrads should appear once, grads twice, and software engineers four times.
If round-robin is not appropriate for your particular review, use your judgement to pick a reviewer on a case-by-case basis.
[edit] Who Is Responsible for What
- The reviewer is responsible for:
- Completing reviews on time.
- Helping the developer find a reviewer if you can't do it.
- The developer is responsible for:
- Choosing an appropriate reviewer, whose skills and availability match the code needing review.
- Ensuring the review process happens on time.
[edit] Mechanics of Review
This is one possible way of work. If you prefer to vary, that's fine (and perhaps you should argue for it becoming the canonical example here). However, your process should ensure that (a) you look at every line of the diff and (b) you have the opportunity to comment on arbitrary lines of the diff.
FIXME: In particular, there appear to be numerous tools out there to facilitate code review. If someone wants to look into them and figure out a better process, that would be wonderful.
- Make a diff to review:
diffbr <BRANCH> > /tmp/review.diff
diffbr is a script to look at the diff of the code. (More details for Grouplens members)
Note: different formats may be useful, depending on the situation. For example, you may wish to view the code itself, or a more targeted diff. Use your judgement.
- Open review.diff in a text editor (e.g., gedit).
- Go through the diff, adding your comments. Put general comments at the beginning. Mark your comments with %%% at the beginning of each line.
- Add yourself to the CC list to the relevant Bugzilla bug.
- Attach the review to the relevant Bugzilla bug. (See Tech:Bug Database for more on using Bugzilla.)
- If needed, discuss the review. Use your judgement to decide whether to discuss on-list, in private e-mail, or in person.
- Assign the bug back to the reviewee. If changes are needed, set the status to REOPENED. If not, set the status to VERIFIED.
[edit] Example reviews
Note: I've obscured the names of the recipients of these reviews. Obviously, it would be trivial to figure out to whom they were sent. However, I think obscuring the names a little makes it easier to focus on the content.
Some of these reviews are fairly critical. However, everyone on the team has received similarly critical reviews; these are just the ones that seemed to be most useful as examples.
