How I review pull requests
Effective code reviews are an essential part of any successful collaborative software engineering endeavour. In recent years, the tool that I have most used for this purpose is also the primary tool that I use for source control in general - GitHub. To that end, I thought it might be useful to document the process by which I review code on GitHub, using the pull request tool. The below is by no means a comprehensive how-to guide, but rather a process that I have gradually evolved over the years in an attempt to cover all the bases.
Important note: This is not a unit test!
I am not a compiler, and reading a pull request is not an integration test. I can only review code well if know that it works, because an integrated test suite has already run and verified that. My job is to check all the non-functional aspects of the code - readability, maintainability, accuracy and portability, test coverage and so on.
Prerequisites
Before starting to review code, it is important to understand the context in which it was written, in particular the requirements it was built to meet and any particular goals the author was attempting to reach. Ideally, this is well-documented in the description of the pull request:
- A clear description of the purpose of the change
- A link to any relevant other places (endpoints, project management tools...)
- Screenshots or animated gifs of any visual changes
You are reviewing the whole pull request, so start by reviewing the description. Read it, make sure you understand it, and if not leave a comment asking for clarification before you go any further.
Only review code once it is finished and ready.
Goals
There are several things that you should try to achieve with a good review, in order:
- Understand each change to the code;
- Verify that the overall approach is sound;
- Identify any issues the author might have missed;
- Add your own knowledge and perspective where appropriate
- Aid other reviewers yet to read the code
But whether we are maintaining an open source repository, and reviewing a code from a passing stranger, or maintaining a private repository worked on by a small team of developers, there is one overarching motivation behind each comment that we add:
Help improve the code to the point where you can defend the change as well as if you wrote it yourself.
With this as your guiding principle, you are ready to start your review.
Read the code
The first thing that I always do is read through the entire code change, loosely following it from top to bottom. The goal on the first pass is to understand the context of each individual change. With luck, the description on the pull request has well prepared you to understand the aim of the change, whether that be adding or removing functionality, fixing a bug or something else. By reading the code all the way through, you can now understand the change within the context of the architecture, and mentally construct the change to the codebase within your head.
Tests are code too
In most cases, any source code change will be accompanied by a test change. I always read these on the first pass too, but then leave them aside as the last part to receive a more in-depth review. The goals for reviewing testing code are slightly different, and you will be best placed to evaluate them for completeness having fully understood the source code changes first.
Search up and down
A code diff is not an essay, with a beginning, middle and end! We are traversing
a graph of changes, and many parts will inter-reference each other - a function
added in one part will likely be called elsewhere, and then referenced in a test,
for example. Be nimble and move up and down the code to understand the inter-dependencies. I
find that I make extensive use of Cmd-F to search up and down for references
to functions and variables, or to look for duplication in logic that could
benefit from being DRYed out.
Make sure you understand all the code before you start picking it apart.
Break the problem down
As Dijkstra once eloquently said, a good programmer is a humble programmer. Much of the discipline of software engineering relies on good practice of strong abstraction, accepting that a complex system can only be fully understood in parts, and eliminating through repeated patterns the need to hold in your head too many things at once. Given such efforts when writing code, it makes sense that we can best review code by applying the same technique.
Expand and contract sections to help focus
Understand one piece of the system at a time. Well-written code in most modern languages will have functions that fit onto your screen all at once - use this to your advantage and focus on each unit in term. If the code is not structured this way, consider leaving a comment to request it is refactored for readability!
Comment immediately
Be thankful that you aren't using a red pen for this! You can easily add comments and then remove then if you later find out they're wrong. I tend to pepper comments throughout, reading them again at the end of the review, and deleting any duplicates or places where I have changed my mind.
Turn one big problem into many small ones.
Give useful feedback
Avoid simply pointing out mistakes. Something that looks wrong can only be remedied if the author is able to come up with an alternative solution. From simple typos to more complex structural problems, always aim to accompany your criticisms with constructive provision of alternatives.
Identify priorities
If there are major issues with a change, try not to get lost in the details. A small whitespace formatting issue is hardly relevant if you believe the code to be fundamentally broken, so focus on the big issues first. Bigger issues will require bigger changes to fix, and so a second round of reviews is likely anyway.
Pay attention to the finer details
If large issues are not present, avoid the temptation to simply wave the code through. Use a closer eye and look for the finer details. Paying attention to small things will also help you spot more subtle mistakes.
Not just negatives
There is no need for a review to devolve into sycophancy, but remember that you can give positive feedback too. In particular, for something that is unusual or new to you, a comment noting that an approach is elegant or original can highlight to others, including the author, that it is worth keeping.
Keep your in-line feedback useful and actionable.
Summarise
The final step in your review is to summarise your points. This is a good time to give an overall comment on the quality of the code and the approach, so carefully consider anything you say here. I like to keep the summaries very short, as they are less directly useful than in-line comments, but if there are wider structural problems that are not isolated to any few lines of code then such explanations will have to go here. Equally, if the code is structurally strong, but you have left a lot of pedantic small comments, this is the right place to balance that by pointing out some larger positives to counter a long list of minor negatives.
Use the summary to frame your review and provide context for your comments.
In conclusion
Code reviews are an essential part of collaborative engineering, and pull request reviews are one great way of carrying them out. Try to make your reviews as helpful as possible, and remember these are a part of your job as an engineer too. If in doubt, imagine how you would prefer something to be explained to you, and use that as your guide. And always keep your comments courteous, friendly and helpful.