Pichat SDK, code review process

This document lists guidelines for code reviews. The main goal of a code review is to catch bugs early, at a time when fixing them is still easier than at a later stage. Secondly, code reviews can be seen as a contract between developers to only check-in code that has run trough basic checks and before problems will affect everyone in the team. The here described code review process is light weight, easy to apply and intends to speed up development.

Before starting a review all of the following checklist shall be true:

• reviewed is only code that belongs together (don't mix new features and bug fixes, etc.)
• code compiles without errors or warnings (exceptions must be clearly communicated)
• code is according to coding standard (exceptions must be clearly communicated)
• code is fully white box tested by developer (this is a must have, but no reports required)
• code tests important assumptions and pre-conditions (asserts for pointers, bad arguments, etc.)
• code doesn't fail already existing unit/regression tests
• design is seen as part of coding activity, design documents have been updated if necessary

Additionally, it's recommended to create a copy of the checked-out project or a patch to be used for the code review.

There are at least two persons involved: a reviewee (person which code is being reviewed) and a reviewer (person which reviews someone's code). Please note, not everybody is suitable as reviewer right from the start. Each person has to take part in a couple of initial review processes himself/herself before being able to act as reviewer for others. The team leader informs who is available as reviewer. In any case the reviewee and the reviewer can not be the same person.

Starting the review:

The reviewee picks one reviewer and gives him necessary information (e.g. description of what is reviewed, location where to find files or diffs that can be applied). Contact may established via email or chat. The reviewee sends a review request with a short description of changes and the userid/name of the reviewer... e.g. like the following email subject: "RR: Added code and more details. Reviewer: userid". The reviewer may decline a review for various reasons and another reviewer has to be found by the reviewee. Once a reviewer accepts he must be available within reasonable short time and the reviewer can not be changed anymore.

Reviewing the code:

The reviewer will verify the code until he/she is satisfied with it. If a review handles unfamiliar areas it is recommended to invite in-house specialists for inspection and technical consulting. Reviews may be done asynchronously (e.g. the reviewer receives all necessary information and looks trough the code alone) or alternatively the review is done together (e.g. the reviewee presents the code on the screen and guides the reviewer trough it). Code is reviewed until it passes, the outcome of a successful review has to be documented with a short email.

Closing the review:

The reviewee checks-in the code after the review was finished successfully and an 'APPROVED' was received per email. Only the reviewed code can be checked-in, no late minute changes! In the check-in message use the descripton of the review request (update it if necessary) and make sure that reviewer plus all inspectors (if you had some) are listed. There can be only one reviewer, therfore use one of the following formats: "Reviewer: userid" or "Reviewer: userid. Inspectors: userid, userid".

The review is completed now, happy happy joy! :)

When you want to make reviewing smooth:

• review more than once a week ('review often philosophy'). Don't review big code drops because it will take for ever, stresses your reviewer and is not as effective as with smaller iterations.
• before sending out a review request look at the to be reviewed code from the perspective of the reviewer (and other persons), is the code understandable, consistent and "ready" to be checked-in?
• a reviewer has the right to decline a review because the code is a "quick and dirty hack" rather than a stable, maintainable work. Missing or bad design is another reason for declining a review.
• prototyped code shall be discarded or rewritten before being accepted into the code base. Always.
• unless the code was really crap when you started the review, tell the reviewee that he did a good job! Give bonus points for good coding style, good design and good ideas.

When something goes wrong:

• don't blame people. Work together to make things better. Meet personally and talk.
• when you get stuck in a review process get immediately a mediator (default is team leader), don't wait. The mediator will negotiate and decide how to continue (in worst case stop the review).
• reviewee and reviewer are equally responsible for checked-in code! Both together will be responsible for handling bug reports and bug fixes. Better chose your reviewer wise.
• checked-in code shall never break the code base, otherwise it will be removed immediately

When you are the reviewer tick off the following checklist before approving:

• check-in message in review request lists what was done (all important changes are mentioned)?
• you are familiar with technical aspects of the code (if not an inspector has been invited/consulted)?
• code is complete and integrates nicely with existing code (possible TODOs have been clearly marked)?
• code has passed existing unit/regression tests?
• your reviewee has demonstrated how he/she has tested the code (if not a new test has been created)?

For further information and feedback please contact me.