zzSimple Code Review Process

http://www.deaded.com/staticpages/index.php/codereviewprocess

 

Simple Code Review Process

1. Overview
A code/peer review is where developers go over the code in a system to:

• make sure that the code is written to standard and satisfies the specifications, requirements or design documents
• suggest improvement opportunities to the author
• learn different ways of coding and about the system under review (competence building)

All code that is intended for production is a candidate for review.

Document base from www.processimpact.com

2. Work Aids
The following peer review work aids are available:

• Issue Log (for keeping track of issues found during the review)
• Java Inspection Checklist (things to keep a lookout for)
• Reviewed Code Tracking (keep track of what was reviewed and when)

3. What to Review
To get some idea which code to review, think about the following:

• code that uses new technology, techniques, or tools
• key architectural components
• complex logic or algorithms
• security related code
• code that has many exception conditions or failure modes
• exception handling code that cannot easily be tested
• components that are intended to be reused
• code that will serve as models or templates for other code
• code that affect multiple portions of the product
• complex user interfaces
• code created by less experienced developers
• code having high cyclomatic complexity
• code having a history of many defects or changes

When starting to do code reviews for the first time, core components, base classes, and complex areas should be started with. As the code reviews progress, more coverage is possible by choosing a use case to follow through or selecting a layer (eg; EJB). Over time, reviewing code as it is made ready and also picking on any areas that keep having bugs reported in them. However, at no point should there be a target to review 100% of the systems code.

4. Participants
Who should take part in a review? In general, the code should be reviewed by:

• the author
• peers of the author
• possibly a 3rd party unrelated to the project

In attendance, there should be at least the author of the work under review, the architect, and at least one other developer.

Ideally, 3 to 5 people should take part, not all needing knowledge of the system under review.

5. Inspection Procedure
The actual review meeting should be no longer than 2 hours a time (and never two reviews in the same day).

5.1. Before the meeting
To start with, someone selects which code will be reviewed. The author(s) of the code ensure that the code is written to the coding standards, compiles, and has passed static analysis checks (PMD and FindBugs).

The code is then printed (one copy for each reviewer). At this point, the code under review is frozen. This means that no more changes are to be made to the code under review until after the review has been completed and any actions taken related to the review have been dealt with. There is no point reviewing code that has already changed, and also no point in trying to fix issues raised by the review in code that has changed since the review.
Printed code must have the class name, line numbers and a page number printed on each sheet.

5.2. During the meeting
Code reviews are for reviewing code – not for going over system design, picking at brace placement or spacing, or criticising the way the author has written anything. Remember that you are reviewing application code – not the person that has written that code.

The review should have a light, informal, atmosphere where questions relating directly to the code under review can be freely asked and answered, yet also keeping the meeting moving forwards.

The author(s) of the code starts with a quick verbal explanation of what the code does and then starts to go through the code. This does not mean in a robotic fashion “line 1 import com.mycompany.bla.bla” … “line 55 for x equals zero, x is less than the size of myarray, increment x”. It means to describe in natural language the important aspects of what the code is doing. The line numbers are used to direct attention to a place in the code when going through.

When an issue is raised, it needs to be recorded in the issue log so that it can be dealt with after the review meeting. The class name, line number, and text description of the issue should be recorded. It is up to the project to decide what should be fixed and when, sometimes it is enough to recognise an issue and ensure that the developers are informed about what or what not to do in the future. Other times, you will find bugs that must be fixed or possibly some refactoring. When something like refactoring is done, you should also consider putting the refactored code under review again.

Filling in the issue log is the only task that should be done with a computer. Trying to review code on a computer screen with 3 or more people doesn’t work – it must be printed. The issue log should be kept even after any actions raised by the review have been dealt with (this may help to spot any patterns so that the static analysis tools can be improved to help find any issues automatically in the future). Note that these tools cannot replace human checking of what is done, they can only check how something is done.

6. Reference Sheet for Peer Reviews

•  
  • Select review participants, obtain their agreement to participate, and schedule a walkthrough meeting. Responsibility: Moderator
  • Author prints a copy of the code for each member of the meeting. Responsibility: Author
  • Describe the work product to the reviewers during the meeting in any appropriate way. Lead discussion on the topics of interest or concerns about the work product. Responsibility: Author
  • Present comments, possible defects, and improvement suggestions to the author. Responsibility: Reviewers
  • Based on reviewer comments, perform any necessary rework of the work product. Responsibility: Author
Participants
The architect / author selects the participants for the review.

Entry Criteria
Architect has selected work to be reviewed and author(s) has prepared the work for review.

Tasks
Deliverables
Modified work product

Verification
No verification of rework is required. Changes are made at the project group’s discretion.

Exit Criteria
The author has made any appropriate changes in the work product.

7. Process Maintenance
Submit suggestions for improvements to be made in this peer review process to the Quality Department ( [e-mail] ).

 

http://www.stickyminds.com/index.asp