Thursday, 18 May 2017

Code review 6: Code Review Flow

In this post I outline the optimal flow for good code reviews. It is important to strictly follow the process to prevent dysfunctional code reviews from taking place and to improve the experience and speed of code reviews for the entire team.

This article does not cover when code reviews should be called in your over all development process. However it applies to any development process that included a code review.

A code review always starts with an artefact that is “Ready for Review”. This could be a feature or part of a feature. It will likely include some code updates, some tests (hopefully all automated) and some documentation for end users and/or developers. “Ready for Review” means that the artefact is reasonably complete i.e. there is enough completed to do something useful and in the case of code, it executes. It is the author’s call as to when they are ready for review.

The author should look for and appoint two reviewers. You can choose to appoint more reviewers, but two should be sufficient. At least one reviewer should be an experienced developer. You may also be required to appoint a “guardian”.

The reviewers must commit to concluding the review as quickly as possible because we value completed work of the author that is closer to delivery to master, over features that are in progress in the reviewer’s backlog today. Hence the review is always treated as the highest priority task.

The reviewers must read and understand the original “Problem Statement”. What is it that the author is trying to achieve?

All supporting material should also be read and understood. The author may have completed some sort of technical report, design specification, whiteboard diagram of the solution etc. These artefacts should be all used to gain an understanding of the proposed solution. However, absence of these is not a deal-breaker for the review if the solution is the correct one.

Next the reviewers should review the tests that are written for the code update. Tests should be well described and their terminology should match the problem domain. Do the tests follow a Given-when-then format? Are the tests correctly coupled? Are they manual or automated? There should be some working tests available prior to reviewing the code.

The author will already have looked at the static analysis tooling and should have removed all the trivial violations as part of getting ready for the review. The reviewers should review any static analysis tooling output. Are there any new failures since the last build? Are there possible hotspots in the proposed solution? Any violations that are entirely reasonable to leave in the code?

Now that we have a good understanding of the problem, the overall solution proposed and all the automated static analysis has been done, the reviewers should review the code. All feedback should be ranked as major or minor and logged independently of each reviewer at this stage. Genuine and honest comments should be recorded with consideration for the feelings of the author.

Next the Authors and reviewers should come together, supported by tooling, and discuss the feedback that the reviewers have for the Author. For each piece of feedback a decision must be agreed as to whether re-work is required or not. In the exceptional cases where consensus cannot be reached, another senior developer should be brought in to arbitrate. It is reasonable to assume that their decision would be final.

The outcome of the review is a three way decision and it is the reviewers’ call. The code and supporting artefacts can be accepted and merged to master. Alternatively re-work is required. The fact that this option could have serious implications for the project awaiting the output, means that this option needs to be handled professionally and with courage by the Author, the reviewers and the team’s leader. At all times we must be aware of our deadlines and take actions that are pragmatic given the time allowed and the implications for the user or customer.

Should minor re-work be required, the author should complete this as soon as possible. A quick glance over the re-work maybe required by the original reviewers to ensure the work is satisfactorily completed, however this is optional and should have been clearly stated by the reviewers at the decision stage.

If substantial re-work is required, the story or task should be brought back to the beginning and any shortcomings addressed using the hands-on help of more senior developers or a mentor. It is preferable to assemble the same reviewers when the code is ready for review again as these reviewers already have an awareness of the problem being addressed. Otherwise it should be considered a brand new code review.

In summary, this article outlines a simple, but clearly defined flow or set of steps for performing excellent code reviews. Following it strictly will ensure consistent and high quality code reviews in your team, resulting in a better product coming out of your team.

Thanks to contributions from Shane McCarron on this article

1 comment:

  1. Appreciate this blog. This blog provide complete information with flow chart about use and importance of automated static analysis tool. Thanks for sharing