Code Review

What is code review?

Code review is a procedure where a colleague of the original developer “proofreads” the edits made within a change. The goal is to keep high quality mainline by quickly catching any inconsistencies, accidental mistakes and other shortcomings the developer might have overlooked.
The results of a review should not be used to assess the developer – the goal is to improve quality, not to spoil relationships.

Introduction

Code reviews have been proven to be an effective way of enhancing quality and sharing experience a long time ago. Code reviews typically discover more problems than testing. Testing is still useful since the areas of detected flaws do not completely overlap.

Code reviews should be performed by experienced developers. For more critical changes or projects, several people might have to review the same code before accepting it for integration.

Reviewing code is very effective for blending in new team members. The “newbie mistakes” and misconceptions can be promptly brought out and discussed with the new teammates.

Since it is the quality of a developer's output that is being dissected in review results, the reviewer must take care to avoid hurting feelings of the developer of reviewed code. It is important to make it clear that the idea of code review is not about attacking him/her, but about achieving common standards, higher quality and less bugs to fix later.

It is important to review every change. First, when a change feels unimportant, it is also less tested, therefore chances of introducing a possibly severe bug are higher. Second, the more broken code slips in, the more will team members be reckless (“hey, it's broken anyway!”) and the less will team members believe in the quality assurance method, thus leading to a vicious cycle of doing even less reviews and having even more bugs creep in.

Since code reviews are most useful when performed by someone other than the original developer of the change, reviewing changes developed by yourself is usually prohibited in Changelogic. This option can be changed on project details page.

How to?

Code review can be performed on changes that are in state “awaiting review”. To start reviewing a change, click “Start review” on the details page of the change. Depending on the workflow of your project, code review may also be done after the change has been tested by a tester. In this case “start review” is done by a tester and if testing of functionality passes, the tester reassigns the change to a code reviewer. The state of the change remains “being reviewed” during such reassign.

During review, you might want to check various aspects of submitted code, such as:

Have all the implementable tasks really been implemented; if not, then why?
Have all the tasks that have been implemented been related with the change; have they been registered at all?
Does the code adhere to various standards and conventions used in the project?
Is the code maintainable, easy to understand?
Does the code do what it is supposed to?
Are all error situations handled? Are they handled correctly?
Are there any security holes?
Are there any algorithms too inefficient for real-life large data volumes?
Has the documentation been brought up to date?
Have any issues been left for resolving later? Have the tasks been added for resolving such issues?
Have the required environment adjustments been added?
Does the planned test release cycle or macrochange of the change cycle conform with the nature of the change?
Have all the new files really been added to the VCS?
Have all the unnecessary files and parts of code been removed?
Does the impact of the change conform its planning?

You may define review templates to avoid neglecting to check for common trivial shortfalls. Based on the template, a form will have to be filled in by reviewer each time a review passes or fails.

The most time consuming part of a review is reading diffs. On the details page of a change, you can see the list of files that were edited, added or removed within that change. Click on the listed filenames to see how the file was edited within the reviewed change.

If you want to check out the whole change from VCS, you may use command ant checkout_version. Should you wish to perform the checkout manually, the tag is given on the detail form of the change along with the command how to update your working directory to that change. Using manual checkout is discouraged as it might not be possible in future versions of Changelogic.

When rejecting a review, be sure to describe thoroughly what exactly drove you to that decision and how things should be improved.

Statistics: according to Changelogic's current state of practice about 30% of overall code reviews fail, usually for several reasons. This translates into quite a number of bugs that would have otherwise reached the mainline.