Code Review Best Practices
I am a firm believer in delivery high-quality code at all times. As such, anytime the committer of code isn’t the only person looking at the code before it goes live, we can be reasonably certain that the quality of the code will increase.
Below are a number of my best practices with respect to code reviews. Note that these are not all-inclusive, and there are many other ways to make sure we do these correctly, but the items detailed below are some of my biggest offenders.
Low Barrier of Entry
This is the first one in the list, and it is the most important. When asking others to review your code, you must decrease the difficulty of reviewing. There are a number of ways to do this effectively.
- Define and use a useful review template. GitHub allows for Pull Request Templates to allow for quick creation of a review. This ensures that the same information will be captured regardless of who starts a review.
- Don’t make the reviewer go somewhere else (like JIRA) to see why you are making this change. Include that section in the review template. All the pertinent information regarding the WHAT and the WHY of the change should be evident on the review request.
- Include anyone that might be needed by default. GitHub has a concept of Code Owners that allows for a default user or team to be assigned to a Pull Request if a defined section of the code is changed. This will ensure that the reviewers don’t need to go hunting for changes to products they own.
Respond to Comments
If a person is reviewing code, it means she is spending her time doing something to help you. This cannot be focused on enough. This person is doing you a service!
With that being said, if you dismiss or ignore her comments, and you close the review without responding to the comments, you are sending a very clear signal to the reviewer: you don’t care what she has to say.
Don’t be this person. This person is someone that no developer wants to work with.
Keep Them Small
There is legimitately nothing worse to review than a branch with hundreds of changes across dozens of files. The amount of time required to accurately review that level of change is astronomical, and that’s with a guaranteed failure to find all reviewable points.
If you find yourself making a lot of wholesale changes across a number of files, consider instead breaking the work down into more manageable chunks. Having multiple reviews that combine into a singular feature branch isn’t ideal, but it certainly is better than having one massive review.
Style Changes are Separate
Remember: code reviews are something that the reviewer is spending his valuable time on. It needs to be effective as possible.
If there are a number of unrelated stylistic changes alongside a functional change, it can be very difficult for the reviewer to sift through the reviewable changes versus the style changes.
When in doubt, don’t make the stylistic changes if you are working on something new. Create a separate branch and review for those style changes, and keep everyone’s attention focused to the functional changes.