View source | Discuss this page | Page history | Printable version   

Code Review Process

Contents

Overview

This document gives a description of the Openbravo ERP code review process and the code review checklist.

The general opinion is that code reviewing is very helpful in finding and preventing bugs and in transferring knowledge between development team members. On the other hand traditional (Fagan-like) code reviews are very time consuming and resource intensive. In addition, standard Fagan like code reviews fit less to more agile-like development methods and current development environments with instant compiling and code formatting tools. The result is that it is difficult, in practice, to organize and implement code reviewing processes in the dynamic and time-pressed environment of a development project.

The Openbravo ERP code review process tries to combine the advantages of code reviewing with a process that is lightweight and easy to organize and control. The process is very much focused on the code itself and uses the features of development environments and code checking tools. Using code checking tools means that code reviewing can be focused on code quality and correctness instead of code standard conformity.

Before going into further detail it is good to define the goals which we want to achieve with code reviewing. There are two main goals:

The Openbravo ERP code review process should be supported by a toolset. Instead of making an upfront choice for a tool, the envisioned process is described. The next step is to determine if an existing tool can be chosen or that a code reviewing tool can be created by customizing current development tools (svn, mantis, eclipse).

But before describing the code review process, lets first emphasize the role of the developer in the software development process. Because high quality code starts and ends with the personal approach and drive of each developer.

Coding is Craftsmanship

In many ways a developer can be compared with a craftsman like a carpenter, mason or even an architect. Coding consists as well of standard plumbing work as high level design and detailed design activities. Coding software is a craft which requires discipline and an eye for detail. The devil is in the detail especially with coding.

It is the developer who is the Author of the code. It is therefore the developer's responsibility to take pride in writing Good and Clean code: code that runs, code that is readable, elegant, easy to read, easy to adapt, efficient. This is especially important with open source software: the code is visible to the world. This puts extra responsibility on a developer. The code represents Openbravo ERP in the open source and ERP world, it is code which is used, seen and reviewed by other developers of other companies on many different countries.

In every few lines of code there are a multitude of design choices which can be made. Even just a few lines of code can be implemented in different ways: smart, elegant, efficient, easy to understand, easy to read and adapt, or complex, abstract, inflexible, bug-ridden. It is the developer who makes the decision which type of code to write. With the many thousands of lines of code in Openbravo ERP it is clear how important this is.

The code review process does not have as a goal to limit the developer in his craftsmanship. The goal is to exchange knowledge, increase capabilities and support developers in making automatic choices for standard structures. This allows the developer to spend time and effort to the things which are important: create elegant, understandable code in a productive way.

Openbravo ERP Code Review Process: Personal, Peer review and Team review

The Openbravo ERP code review process consists of three types of reviews: 1) Personal, 2) Peer and 3) Team reviews.

Personal Review

The developer should automatically follow coding conventions and other good coding practices. To validate this each developer should actively review his own work. The development system should support the developer in performing this task.

The following process is proposed:

The personal review should not be done on the same day as the code was written. A longer time between the commit and the personal review will result in a fresher look on the changes made the person himself.

Peer review

The peer review is the most important review as it operates on two goals: issue prevention and knowledge transfer (from the reviewer to the reviewee).

An important question is who is the peer, who will review the code. In the Openbravo ERP code review process the peer reviewer is the team lead of the scrum team.

The advantage of the team lead peer is that he understands the functionality the team is working on. He can therefore judge if changes are correct in respect to the functionality the team is working on. In addition the team lead has direct contact with team members. Also code reviewing is more part of the standard team activities and can be planned. As the team lead is an experienced developer, code reviewing results in knowledge transfer from the team lead to less experienced team members.

The team lead peer approach has as disadvantage that the team lead might get overloaded with review activities. Another disadvantage of a team lead peer is that he might feel less ownership for overall source files and can possibly not determine if code changes interfere with other (unchanged) parts of the code.

The commits by the team lead himself should also be reviewed. Therefore each team lead also has a personal reviewer assigned to him.

The peer review process consists of the following steps:

All peer reviews should have been done before a branch can be merged with trunk.

Team review

Each scrum team should spend at least 45 minutes a week on a team review. In the team review one (part of) a file is discussed with the complete team together. The main purpose of the team review is to share knowledge about code and coding practices. The to-be-reviewed file is proposed by the team lead. The team lead informs the rest of the team which file will be discussed and each team member spends 15 minutes for doing a personal review of the file.

In the team review meeting the author of the file presents the code and gives a walkthrough. The review meeting should be an exchange of ideas and discussion of implementation strategies. The goal is to share ideas and knowledge and not so much to find bugs or non-conformance to standards.

What to Review

The following files are reviewed:

Only committed changes are reviewed. This means that everyone is free to commit without pre-review. This is as it is now. The commit is the trigger of the review.

However not all commits need to be reviewed. The following rules apply. Every commit is reviewed, except:

Different types of files will require different types of review checklist. The proposal is to have different checklists for different files:

The checklists are discussed below in more detail.

Review Statistics

For management purposes the code review system should register and display the following information:

Code Review Tools

There are different tools available which can be used and adapted for the flow described above. If the decision is made to use an external tool then a proof-of-concept needs to be planned to try one out.

A choice can also be to implement and support the above flow with a custom developed web application integrated with svn and Mantis. Mantis can be used as the registration of review tasks and results. For example a Mantis bug can be used to start a review task. A rejected review can create a Mantis issue assigned to the committer. The result of a review can be attached to the Mantis issue.

The advantage of developing our own toolset is that it prevents yet-another-tool in the development and systems management environment.

Here you can find a template sheet to perform code reviews.

Automated Reviews and Bug identification

The manual review process is supported with the following tools which perform automatic code reviewing:

Checklist

There are different checklists for different situations. Each checklist is short to be effective, referring to external documents for more information.

A list of topics to review can be found here.

Short Checklist (Java)

Full Checklist (Java)

Properties Files

Application Dictionary Checklist

Function, Trigger and Views

[TBD]

Links

Appendix: who is the Peer

The main review activities are done as part of peer review. An important question is therefore who is the peer who will review the code.

The Owner Peer

The Owner Peer approach means that the source code of Openbravo ERP is divided in different parts and each part is assigned to one owner of that code. The owner of each part is responsible for reviewing the changes to the code made by other developers.

The owner should be someone with experience and an eye for code quality. He should have full understanding of the coding conventions. In addition the owner should understand the functionality implemented by his part of code. He should feel responsible for his code.

The owner peer approach has as advantage that the reviews are done by a person knowing the overall code (of his assigned part). He can detect when code changes conflict with the broader meaning of the code. Also because the owner peer is an experienced developer he can be more effective in sharing his knowledge with less experienced developers whose code changes are being reviewed.

The main disadvantage of an owner peer approach is that the owner peer is not involved in every project/scrum team that makes changes to his part. Therefore it can be more difficult to understand/follow the reason why code changes are made.

The Team Lead Peer

The team lead can also act as the peer. The advantage of the team lead peer is that he understands the functionality the team is working on. He can therefore judge if changes are correct in respect to the functionality the team is working on. In addition the team lead has direct contact with team members. Also code reviewing is more part of the standard team activities and can be planned. As the team lead is an experienced developer, code reviewing results in knowledge transfer from the team lead to less experienced team members.

The team lead peer approach has as disadvantage that he might get overloaded with review activities. Another disadvantage of a team lead peer is that he might feel less ownership for overall source files and can possibly not determine if code changes interfere with other (unchanged) parts of the code.

The Team Member Peer

In this approach the work of a member of a team is reviewed by another member of that team. The advantage of this approach is that team members exchange knowledge and that team members together grow in their experience and knowledge level. Doing code reviews can help to more quickly develop developers skills. Another advantage is that the review work is divided.

The disadvantage is that also inexperienced developers will do reviews which will probably result in less effective reviews.

The Second Peer

Also the work of a peer reviewer should be reviewed. Therefore every peer (team lead or owner) should have a second peer assigned to him.

Retrieved from "http://wiki.openbravo.com/wiki/Code_Review_Process"

This page has been accessed 8,801 times. This page was last modified on 7 March 2017, at 08:00. Content is available under Creative Commons Attribution-ShareAlike 2.5 Spain License.