In my role as a senior developer, I review a lot of code. In fact, there are many days where I end up spending more time reviewing others’ code than I do writing code myself.
When done well, code reviews can add extraordinary value and help prevent bugs from ever getting checked in. On the flip side, reviews can consume hours of developers’ time, delay the completion of tasks, and even sow divisions within teams.
In this post, I want to talk about the code review process and share some advice to help make your code reviews as effective and efficient as possible.
Purpose
Before we dive in, it is useful to think about the purpose of code reviews. Identifying and resolving any bugs in new code is certainly part of it, as the earlier you uncover a bug, the less damage it will do. But beyond that, reviews are really meant to foster discussions to help the team land on the best approach.
As such, review comments should never be a mandate on how things should be done. For reviewers, this means being objective and providing a reason for any suggestions made during the review. For developers, this means asking questions if you don’t understand the justification for a change. Sometimes things may be more of a subjective call or a stylistic preference where there is no good reason to go with one approach over another, in which case the team should defer to the original developer.
For Developers
Ultimately, the success of the code review starts with the developer. Their attention to detail and adherence to the team’s process will make all the difference in how long a review takes.
As a developer, your goal should always be to submit code that gets approved the first time. Don’t accept the mindset that review comments are inevitable. Always do your best work and try to learn from comments on previous reviews.
Before you submit your code for review, ask yourself these questions:
- Does my code adhere to my team’s coding standards and guidelines? Reviewers shouldn’t need to bother checking for rules that the team has established. These standards should be clearly documented and easily accessible for all developers on the project.
- Have I resolved ALL compiler/IDE warnings? Use suppressions sparingly for false positives.
- If I read my documentation comments out loud, do they make sense? Plenty of typos and grammatical mistakes can be discovered this way.
- Did I write unit tests for new code and verify adequate test coverage? Consider integrating the JaCoCo plugin into your build pipelines to automate this step!
- Did I test my changes at the system level? Should go without saying, but make sure you’ve verified your code by testing it on the system, even for the smallest of changes.
- Is the branch submitted for code review up to date with the target branch? This responsibility falls on the developer to help avoid unexpected merge conflicts.
- Did I perform a self-review using the diff on GitLab/GitHub? This is what the reviewer will use when reviewing your code, so make sure it looks as you would expect.
- Is this the best code I have ever written? If you are not proud of your work, it is not ready for review. Don’t use reviews as a crutch to catch your mistakes.
For Reviewers
Reviewers should begin by going through the same set of questions, double-checking to make sure nothing has been forgotten. Even though the developer should have taken care of these items, we all forget things at times and the reviewer can help catch any omissions.
From there, the biggest piece of advice I can give to reviewers is to review the code in the IDE. Yes, that means cloning the repository if you have to and checking out the branch. IDE inspections will catch all sorts of errors that someone reading the diff might not catch. Often, an innocent looking change in one place leads to an unused variable or method somewhere else that needs to be cleaned up. Even though diffs on GitLab/GitHub have some syntax highlighting, reviewing in the IDE will give you more context for the changes and help you spot things you might have missed otherwise. I always have the diff on one monitor and the IDE on the other as I work through a review.
Conclusion
Adhering to these guidelines, developers and reviewers can cooperate to streamline the code review process and drastically improve the quality of your code. It’s always easier to make changes to code before it gets merged than after, so make sure to use reviews to your advantage and make sure they always serve the code!