Maintenance Matters: Code Reviews
In this entry of our Maintenance Matters series, we'll cover code reviews: an integral part of the cycle of developing code.
This article is part of a series focusing on how developers can center and streamline software maintenance. The other articles in the Maintenance Matters series are: Continuous Integration, Code Coverage, Documentation, Default Formatting, Building Helpful Logs, Timely Upgrades, Good Tests, and Monitoring.
Code review isn't all just catching bugs and LGTMs. It improves readability, helps keep everyone up to date, and is a great place to learn and discuss things that don't have obvious answers. Multiple sets of eyes mean that we write better code.
What are code reviews? #
The code review process is one in which a developer or team of developers reviews code before it is integrated into a codebase.
Why do we do them? #
- Identify potential problems
- Ensure that the code adheres to coding standards and best practices
- Improve the overall quality and maintainability of the codebase
- Learn new coding techniques
- Share knowledge and expertise among team members.
How and when are they done? #
Code reviews can be performed using various methods, including pair programming, informal walk-throughs, and code inspection. They should normally be done before changes have been merged into the codebase.
At Viget, code reviews often take the shape of a developer requesting another developer (or the team) to asynchronously inspect the changes made in the pull request, leaving comments and questions commonly on the pull request itself.
A code review comment may look something like this:
The overall goal of code review is not to criticize the individual who wrote the code, but rather to ensure that the code is of high quality and meets the standards and expectations of the team. While peer feedback can (by its very nature) be anxiety-provoking, it's an opportunity for collaboration and learning, and can help improve the overall quality of the software being developed.
Who should review code? #
Everyone! It may be easy to assume more senior developers should be the ones who review code, but that's not always the case.
While feedback in code reviews is an excellent way for junior developers to learn from senior developers, there is still a lot that can come from a junior developer reviewing a senior developer's code. The junior may know about a new technology, methodology, or simple trick that the senior may not know.
Additionally, the act of performing code review is itself a learning experience. The junior developer will learn a lot about the codebase and the project as a whole by reviewing the code of others. The same can be said for a senior – learning should never stop!
Finally, a fresh set of eyes on a problem that the code author has been working to solve for a long time is going to be helpful more often than not, regardless of the experience differences between reviewer and reviewee.
Some things Viget does #
The Viget development team has some (in my opinion) unique methods during code reviews that I wanted to share.
The conditional approval #
The development team has built up a large level of trust in one another, and what I dub as "The conditional approval" is a good example of that. Basically, it's when the code being reviewed does need changes, but they are minor enough that the feedback is given with an approval. Note that this may be specific to how GitHub handles pull requests and code reviews, but the general flow is like so:
- The reviewer reviews the code and comments "Generally LGTM (looks good to me). Could you change this field to be a
BigIntegerFieldrather than just anIntegerField?" along with marking the pull request as "Approved." - The pull request author makes the changes requested, and commits the new code.
- Assuming the pull request only needs that one approval in order to be eligible to be merged (and the code passes CI), the author can then freely merge the code without needing to request another review. This allows for there to be less downtime - waiting for the same reviewer to get the notification they need to re-review the same pull request - especially if the requested change was very small.
Automated code review bot and Slack channel #
We have a dedicated Slack channel, #code-reviews, in which a bot tracks new pull requests opened in the Viget organization and posts a message in the channel. This allows developers insight into new code that needs reviewing, and takes some of the burden off of the code author to manually assign reviewers to perform code review.
Assuming positive intent #
Code reviews at Viget are always meant to be taken constructively. We encourage code authors to read through a code review given by a peer with positive intent. This helps the code author get out of a defensive mindset, along with encouraging the code author to bring positive intent to any code reviews they will do in the future.
So what? #
Having a strong and well-understood code review process helps applications be maintainable. It is much easier to contribute to an existing project and keep it afloat when the codebase is properly reviewed. That code often has fewer bugs, is more readable, and the simple back and forth of a review helps both the author and reviewer learn more about the project and programming as a whole.
The next article in this series is Maintenance Matters: Good Tests.
