Effective Code Review Practices for 2023

The Code Review life cycle - an overview

From pushing the first lines of code to deployment, the software development cycle involves a pipeline of tasks, processes, and tools. Consider a feature implementation task which involves adding new lines of code - once the engineer is satisfied with the work and is reasonably sure about meeting all product requirements, a Pull Request or a PR is raised. 

Depending on the Git provider, it can be referred to as a Pull Request (Github), Merge Request (Gitlab) or Diff Review (Phabricator) - all of which are code review requests.

This initiates the review process, where peers or managers are requested to review the changes made, suggest improvements, edits, or retractions and finally give the green light for this set of changes to be shipped.

Why should PRs be reviewed?

Any and all changes to code are added only to a secondary branch whose purpose is to contain changes that are relevant only to the task under consideration. This is later merged with a larger branch, usually called the `main` branch - considered the production software branch that is deployed everywhere. This practice ensures that the changes being added to the software:

1. Are complete in their feature requirements,
2. Meet code quality standards,
3. Passes test cases with acceptance criteria

A Pull Request or a PR is then raised to merge this secondary branch onto the main to deploy the changes to production. 

Completeness of the feature

The code review process in a Pull Request begins with a completeness check that is critical in ensuring that the code changes cover all the requirements detailed in the associated work item. At this stage, the reviewer also makes sure whether the PR is linked to a ticket or not, potentially pointing out loopholes in the development process. Note that all changes to code can become enigmatic without the details on why, when, and how they were implemented that are usually found associated with the associated work item.

Code Quality checks

Anti-patterns: Anti-patterns are code patterns that tend to negatively impact readability, compile times, bug rates, or overall quality of the system. These can be as simple as using inefficient algorithms or not using language specific constructs, paradigms or patterns, often stemming due to the engineer’s lack of experience. Or, using logic or pieces of code that are generally flagged by malware detection softwares. Identifying these might seem like a daunting task at first, but thankfully they are not. They are rather easily identifiable by an experienced reviewer. And for more junior reviewers, there are free tools available that help avoid or detect mistakes that are costly. Code Reviews are a crucial process in maintaining a code base that is free of anti-patterns which in turn maintain and improve overall code quality.

Code readability and complexity: Software dev is a team sport, involving several engineers working on a shared code base. This makes it imperative that code is readable, because most time will be spent reading, understanding, and analyzing code and the solutions to the problem and less time actually writing code.
Laurie, a senior software engineer from Netflix, tweets the importance of reading code thereby throwing light on the readability factor of the code base:

Readable code also helps in code reusability and is faster to detect and fix bugs.
Complex code that is harder to read can mean it's harder to maintain in the long run and can impact delivery times in the short run.
Code reviews check that code is readable and is not too complex. Code reviews also help keep bugs at bay and ensure that reusability is prioritized.

Documentation: Documentation within code in the form of comments that explain the reasoning behind the implementation, design, architecture, etc., is crucial. It forms a very important aspect of readability.

Additionally, external documentation in the form of style guides, testing guidelines, setup instructions, IDE/code editor setup guides, directory structure, and more help keep the project organized, understandable and, most importantly, give specific details on how to fix, expand, alter or otherwise maintain the code. Because these types of documentation are yet another mandatory checklist item during the review, it becomes important from the perspective of improving and maintaining code that reviews be conducted thoroughly.

As an extension of the documentation being in place, the code review process also includes making sure any and all code that’s committed follows the accepted rules and guides for a given project. This again is a crucial part of maintainability of code and hence shows the importance of reviewing PRs.

Testing

Any and all changes to the code should go through rounds of testing, be it unit or integration testing, or even manual testing, ensuring that the changes don’t cause unexpected behaviors. Every PR should include a complete set of test results after one review, and if the results are not within acceptable ranges, a rework should be requested from the author.

From shipping bug ridden software as a result of insufficient testing, to shipping hard to maintain code, or incomplete features that cause bad customer experience, reviewing PRs is one way to ensure these risks are minimized.

What to measure to ensure a healthy code review process

Involvement

An important metric to track is the extent to which the team members are reviewing each other's code. It is measured both as the average percentage of PRs reviewed per team member and the percentage of teams that have reviewed any PR. Having most of the team doing regular review of each other’s code is important not only to keep the code review load balanced but also to ensure each developer has a chance to be in touch with different components of their codebase. Therefore involvement metrics greatly impact cycle time and review quality.

Reaction times

A pull request can have the tendency to illuminate communication gaps in teams. This can be seen in the form of delayed responses, delayed reviews, or delayed acknowledgements. It can also be in the form of long drawn out discussions on rework suggestions/requests. 

💡 Managers should keep a close eye on the average times in a PR like average reaction time and average time to review and identify patterns that might be hampering the review process. Managers can observe these average times by keeping track of the number of back-and-forths in every PR and identifying bottlenecks in reaction times and communication behaviors within the team. Time to react based on test results, number of failed tests, and the rework times based on test results are additional metrics to watch out for in PR reviews.

Rework percentage and Risk metric

Code rework (a.k.a. code churn) is code that is changed within 3 weeks of the first merge. The percentage change of code in terms of rework can throw a light on the amount of change being committed to the code. Change in code is common but it can be a reason for concern. This is because the same module of code being churned multiple times can lead to an increased risk from the subsequent changes - making code churn or rework % one of the most important metrics to watch out for in a PR Review.

The amount of change being committed to the code determines the rework percentage. It’s also been shown that the amount of change introduced has a direct correlation with the risk being introduced to the system. This means, larger PRs that have a higher rework percentage tend to carry higher risks which not only means the pickup time by the reviewer will be longer than normal but also will need more due diligence.

Some changes might be inherently more risky than others depending on the code being changed, for example, changes in auth flows can be high risk as they tend to have ripple effects throughout the system. This means identifying and reviewing special modules is a matter of high importance in PR reviews.

Additionally, PRs that are larger with a high amount of rework tend to be inherently risky in terms of causing bugs in production. This metric can help teams prioritize reviews especially when there is a high review load for senior members. One other metric to watch out for in PR reviews is “Time to Pick-up” by the reviewer as it can vary depending on the size of PRs, and can be high when the PRs are large.

Time to Merge

The average time between creating a code review request or a PR to it being merged to the main branch is called Time to Merge. This indicates the average time the PR was kept open, which can throw light on communication and collaboration patterns, code quality, and testing quality. Additionally, changes in the specs of the task being worked on due to revision in customer requirements can lead to scope creep which obviously increases the time to merge.

💡 Teams should strive to keep time to merge under a reasonable time and should identify the steps under the review that are impacting Time to Merge and identify solutions to these problems. Metrics like reaction time, number of rework requests, number of test cases passed, and number of revisions in task and their timelines can shed some much needed light on why the PRs are kept open for longer than desired times. 

Engineering analytics as a solution

The pull request review process is a crucial cog in the development life cycle that requires metrics that are tailor made to help managers identify anomalies quickly and act on them swiftly. Hatica is an engineering analytics platform that provides metrics like cycle time, rework time, code churn, and more. These metrics can help managers continually monitor crucial metrics, adopt goals to set standards and schedule reports that keep you informed on email or slack.

Request a demo to know more →