Sunday, June 24, 2012

How to set up Code Review Process using Atlassian products

The Source Code Review is an activity which is commonly in a long list of "good to have" things. There is the arguments about lack of time and resources for review. But the aim of the review process is to improve the quality of the source code and to decrease the risks for the project. Is the quality worth of that efforts? Definitely!

Without a code review a development team member may loose the understanding of a high level architecture and produce a bad code which is hard to support, maintain and change in future. In the code review process the new component will be critically considered and evaluated. Two pair of eyes is always better than the one. By the way, in Google company a developer in order to be promoted may spend approximately 20% time on the source code review.

Basic facts

Source code review – the ongoing activity when one development team member reviews a code by another developer. If code is not good enough, it returns to the author with comments and needs to be fixed. If code is OK, than it can go to next phase of the development process.

Author – the developer who wrote the piece of the code.

Reviewer – a developer, architect or consultant who is responsible for review. Any member of the team with necessary knowledge can make a review, but not Author of the code.

Review Coordinator – a project manager or a team leader who is responsible for the process of the review. He or she ensures that the process was initiated and that all code for each particular product release was reviewed.

Changeset – the set of individual changes in the source code or the new pieces of the code which have been grouped for review. It is a lower level logical block for the process of review. Commonly, the Author creates the changeset for the code pieces submitted to a source code repository.

Source Code Management (SCM) tool – the automated tool which stores the source code of the product (i.e. Subversion, GIT, Team Foundation Version Control, etc.)

Project or issue Tracking (PT) tool – the automated tool which stores the project tasks or issue tickets and allows to track, to assign and to watch it (i.e. JIRA, TFS, etc.).

Starting point (model AS IS)

The implementation of the source code review assumes that the development process was already set up or that you have correct understanding of how to set it up. So, you need a team which develops some product based on requirements formalized in PT tool, commits code to SCM tool and prepares some product releases.

Development process

What we are trying to achieve (model TO BE)

The source code review process can be implemented as an additional activity for development process. It is required that each team member knows how to support this activity and makes extra work to do this.

 

Source Code Review Process

Rules of review

The following rules should be taken into consideration when you set up the process:

  1. All code changes should be reviewed
  2. Author is responsible for assigning and chasing reviewer(s)
  3. Review should be done thoroughly
  4. Matching requirements, absence of bugs and good technical design are the primary goals
  5. Coding style have lower priority
  6. If Author and Reviewer couldn't agree on some question they can escalate this to another team member

When reviewing a code you SHOULD follow the review criteria:

  1. Detection of security issues and known vulnerabilities pertaining to the technology stacks used (malicious code, backdoors, Trojans, stack/buffer overflow, etc)
  2. Architecture and design consistency
  3. Detection of non-functional defects (performance, scalability, etc)
  4. Specification coverage (if you use specifications)
  5. Detection of functional defects
  6. Compliance to the company’s IT Policies and Standards

These criteria CAN be applied but with lower priority:

  1. Compliance to Artemis coding standards
  2. Compliance to backward compatibility requirements

Atlassian products to implement the solution

Atlassian offers a full set of the products to support the development cycle and the review process. The good thing about this products is that if offers a complete platform to automate the entire process. They allow you to have an enterprise level solution out of the box.

The main products are: Atlassian JIRA as a PT tool, Atlassian Crucible as a Source Code Review tool, Atlassian FishEye as an integration tool for SCM tools. So, the entire sample stack is presented on the following figure:

Integration of the products for source code review

I will present a complete real life example of how these tools can be used to maintain the process. Please note that all instructions and pictures are for reference only, so it can be different for your process and environment settings.

Complete sample

When the requirements from Service Strategy level are formalized as a task tickets in JIRA a development starts. The development may be organized according to any available methodology. It will not affect the fact that the developer should commit code changes in SCM tool and initialize a code review process.

When the coding is done the developer (Author) commits changes to SCM tool with the JIRA ticket number in the comment.

GIT Commit

Than the Author moves the JIRA ticket to ‘Code Reviewing’ state. Please note that JIRA ticket lifecycle should be adjusted to that process and you need to define additional states for your tickets.

If the integration of JIRA with SCM tool is set up (using FishEye plugin) than on source tab of the ticket you will see the commits which contains the JIRA ticket number in the comment. The review can be started directly from this tab or from Crucible.

JIRA Source Tab

JIRA allows you create a review from the multiple commits. For complex cases you can use Crucible interface itself.

JIRA source tab, multiple commits for one changeset

While creating a review you need to assign the Code Reviewer to the review. It can be done using ‘Suggest’ button. Please note that the Crucible shows only users who have logged in at least once. So, you need to ensure that all team members have visited a Crucible earlier.

It is expected that multiple team members can review the Author’s code. So it can be done by somebody who is available. 

FishEye create review

After the filling of required fields you can start a review. Depending of settings of Crucible you may need to notify the team members about new review. It is good practice to send an IM message to person who is available for review right now.

FishEye start source code review

Than the Reviewer plays his or her role. The Reviewer can comment any line of code just clicking on it. He or she can contact the Author to clarify the unclear code. When current step of review done the button ‘Complete’ should be pressed. The Author will be notified. The Author can add extra commits to changeset in order to meet the comments of the Reviewer. This process can be repeated several times. 

FishEye source code review reviewer comment

When the Author and the Reviewer are agreed on the results, the Author can finish the review by ‘Summarize’ button and move the JIRA ticket to the next state (i.e. Done, To test, etc.).

FishEye summarize review

The Review Coordinator later can check whether all commits have been reviewed. It can be done using the ‘All Open Reviews’ page on Crucible project page to find the open reviews. To find the commits which are not reviewed you can use ‘Search Tab’ in FishEye to execute EyeQL query. For example:

select revisions
from dir "/"
where
(not in any review)
and
on branch ${Place your branch here}
order by date desc
group by changeset
return path, author, date, csid, reviews

So, here it is. Your comments are welcome! Thank you!

4 comments:

  1. Thanks for the post, Pavel. It's good to know how to implement code review with modern tools.

    However, there is one important aspect from code review which you haven't mentioned - knowledge sharing. Rotation of reviewers on regular basis allows both an Author and a Reviewer to learn from each other. Especially if they're collaborating with each other during code review by having a Skype session or just sitting next to each other.

    ReplyDelete
    Replies
    1. This is a good point! The knowledge sharing is additional benefit of the SCR. The knowledge sharing approaches is a theme for the future posts. Stay turned!

      Delete
  2. can you also suggest which all stages shall be created to enforce this policy...And is it possible to automate the task of review coordinator, I mean a task should not be moved to next stage unless all the code review comments get answered.

    ReplyDelete
    Replies
    1. Well, commonly it is not so formal process. The quality of the code is not a formal thing. That is why commonly developers interacts with each other to make a code review.
      To enforce more strict code writing standards there are other tools like style and code analysers (I used StyleCop and FxCop).
      If you would like to enforce the review policy than you can have a separate state for it in your issue tracker of choice (in Jira for example). This state can be changed only after all change sets are reviewed and only by the review coordinator. That is what I mentioned at the end of the article.

      Delete