Code Reviews: A Tale of Two Developers

Code reviews, as one of the tenets of Extreme Programming (X.P.), are fully embraced by our team. The code is never merged directly to either the development or master branches. It means that all code deployed to either staging or production servers have gone through several layers of human checks.

We live by a simple rule: there are no exceptions. No matter what the type of code being developed, whether it’s a feature, a bug or a chore, at least one developer — and often two developers— must approve the changes before it gets merged.

To make sure that enough time is allocated to code reviews, everyone on the team understands that reviewing the code of others is as important as developing his/her own code. So when it comes to tasks prioritization, code reviews are as important as finishing or starting a new sprint backlog item.

Why going to such great lengths? Benefits of code reviews would completely deserve its own article and have been covered in great lengths by many. But for us, the main benefits are:

• Catching implementation concerns of diverse nature: code architecture, security, inconsistency with the existing codebase, etc.

• Collective code ownership: everyone on the team knows the codebase in details and validates each new piece of code. As a result, anyone can spot issues or fix a bug when it appears.

• Enforcing code standards: humans checks allow to both enforce existing code standards and see the emergence of new ones.

But code reviews have also its critics. The main argument against is the considerable investment in preparation for the review and execution time. But in our views, it comes to inefficiencies in the review process.

First, automation can help in shortening the time required to review code syntax issues. We use extensively code linters and analyzers for both web and mobile applications. Pronto is hooked up with several plugins to analyze Ruby-based applications. As for mobile, SwiftLint is used for iOS and checkstyle (Java) / detekt (Kotlin) for Android.

But most importantly, some ground rules need to be laid out between all parties involved in the code reviews in order to get the most benefits. Not only for the reviewer but also for the reviewee — the two developers mentioned in the title. That’s what we will review in details in this article 💪

The Reviewee

The first step is to open and setup properly a pull request for the branch in needs of a review. The “opening” part is easy as most code versioning platforms like Github, Bitbucket or Gitlab make it simple. But the “proper setup” often does not get much attention.

If a pull request looks like the above image, then you have a problem. A pull (or merge) request is the process in which the developer who did the code (the reviewee) asks for another developer to review his/her code (the reviewer). So when asking someone else to do something, it’s necessary to give as many details as possible and inform them about it.

At a minimum, we usually include the following pieces of information:

• The name of the pull request must contain both the user story ID and title:

[<User Story ID>] As a user I can view the list of jobs

• At the top of the description field, add the link to the user story to make it easier for reviewers to get the complete requirements.

• In the description field, include details on the nature of the changes, how to test the feature or reproduce the fix, and add proofs of the work done (screenshots, videos).

• Assign the reviewer(s) to the pull request. That will inform the assignees that the pull request is ready and/or needs to be reviewed regardless of the completion status. In our team, work in progress (WIP) pull requests are encouraged to both get early feedback on implementation and share it with the team. For instance, other team members could cherry-pick code from the branch even though it is not merged. To distinguish these pull requests from the others, the title of the pull request is edited accordingly:

[<User Story ID>] As a user I view the list of jobs [WIP]

With complete details added to the pull requests, half of the battle is already won as the reviewer is now armed with the right information and tools to perform the review properly.

Once the code reviews have started, the reviewee still needs to be tightly involved. Again, there need to be some ground rules so that the comments generated by the reviewer helps in improving the implementation. So the reviewee needs to keep track of the following:

• Respond to every comment either offline (in person) or online. Don’t ghost anyone — except Kenny though as he won’t reply 👻

• Seek to understand the reviewer’s perspective. Assume the best intention from the reviewer’s comments. We are all code owners so everyone involved wants the best for the application.

• Be grateful for the reviewer’s suggestions e.g. Good catch. I will make that change .

• Push commits based on rounds of feedback as isolated commits to the branch. As a good practice, reply to the comment with a link to the fix e.g. Fixed in a4994ec .

• Extract some changes and refactors into future stories if the requested changes are large and/or out of the initial scope of the pull request.

It’s very clear that the reviewee has lots of responsibilities to make the pull request a success i.e. getting the code merged. Clear and constructive communication is the key to get through a code review like a champ.

The Reviewer

Once a developer has been added to a pull request, he/she becomes the one who will ultimately approve to have it merged. While code merges are reversible, it’s quite uncommon. So the norm is that the merged code becomes part of the codebase. It’s a serious thing.

So reviewers need to balance lots of responsibilities that can be boiled down to two areas:

• Making sure that the implementation achieves the code architecture and business goals.

• Understanding the ins-and-outs of the implementation so that he/she can intervene on it without the help of the author of the code.

With such critical purposes, it can be a daunting task to be a good reviewer. What is a good comment? How can I tell the reviewee that the implementation has issues in a meaningful way? What if I don’t understand at all a piece of code?

We solve most of these common concerns living by these common rules:

• Code reviews are not battle-fields. Accept that many programming decisions are opinions. Engage a discussion and reach a resolution quickly.

• Comment directly on the line of code concerned in the web interface of the version control service provider e.g. Github, Bitbucket or Gitlab. If a similar issue happens in many places, avoid repeating the comment. Leave a comment once and mention in that comment that the reviewee should correct the issues in all the other places.

• Use an inquisitive tone, do not make an order:

// Not great 
@user_id Change this variable name to "NEW_VARIABLE_NAME"  

// Better 
@user_id What do you think about naming this "NEW_VARIABLE_NAME"?

• Don’t hesitate to ask for clarification e.g. I didn't understand. Can you please explain? The implementation needs to be crystal clear for you.

• Avoid selective ownership of code:mine, not mine, yours. Remember that code is owned by the whole team.

• Seek to understand the author’s perspective. He/she likely has a broader understanding of the user story as they have worked on it for a longer period of time.

• Identify ways to simplify the code while still solving the problem. It comes down to the wisdom of the crowd i.e. pulling ideas from many people is always better than the ideas of a single person.

• Provide timely reviews. Pull requests should be reviewed within the same sprint to avoid becoming stale or requiring lengthy rebasing thus making them hard to merge. It’s also ok for reviewees to ask for a speedier code review. So always provide an ETA when you will be able to finish the reviews.

Reviewers should only approve a pull request when they feel confident in the code and its impact on the project. They act as gatekeepers thus should take their role with the responsibilities it comes with.

But at the same time, being a reviewer also comes with many benefits. Reviewers get an understanding of all the areas of the application — even on those they did not directly work on — and acquire new knowledge from the implementation of others. Code reviews are a powerful mean to improve your technical skills by seeing how others code.

Conclusion

Code reviews are not a silver bullet by themselves. It’s not because the code is merged via a pull/merge request that code will be better by itself. Instead, a code review is a process in which the reviewee and the reviewer have their own set of responsibilities.

Just like any process, it needs to be transparent for everyone. At Nimble, we use our knowledge base “Compass” as the point of reference for everything we do. As code reviews are an essential part of our development process, most of the ideas shared in this article are our actual rules we live and work by. So we feel confident that it can be applied to any team.