How to do code reviewsLeave a Comment
Peer review is a well established concept in the scientific world, leading to higher quality work, improvements to techniques and an avenue for individuals to learn by examining the work of others.
Following those ideas, code reviews are an important part of our approach to engineering at Server Density. With hundreds of teams relying on our product for mission critical infrastructure monitoring, it is important that we have a robust development process that can deliver on a high level of code quality. Not only that but code reviews are a great opportunity for our team to learn new approaches and understand the decisions behind engineering choices.
That said, there are challenges. In how much detail should you approach a review? How do you provide constructive feedback? How do you deal with an implementation you might not agree with? How do you review your manager’s code?
This is a look behind the scenes at how we do code reviews at Server Density.
Opening the review
All our code reviews are conducted by using Github’s Pull Request functionality. We do all our development on branches, with master always being considered stable. Opening a pull request initiates the code review process and we use a template to provide instructions for the reviewer. A number of key questions are asked by default:
- What does this PR do? A brief description of what is actually happening, and usually a link to the JIRA ticket that is associated with the piece of work. This helps to provide context about the purpose of the code.
- Where should the reviewer start? It’s often not obvious where to begin so this should be explicitly stated. It is typically a particular file and may include pointers to direct the reviewer to the starting point to begin their review.
- How should this be manually tested? The reviewer might not be familiar with this precise area of the codebase, or how to set up the right environment to verify the test case. This is where commands will be listed for the reviewer to execute in order to set up the test, and run the code. Listing precise steps to test the code may be appropriate but often it’s general functionality that is described, leaving it up to the reviewer to step through the changes. This avoids any bias by the author only describing how they have tested it which might miss key elements.
- Who should review or be notified? Sometimes you might want to ask a specific person to review the code but often this is a group of people e.g. the entire backend engineering team. Anyone can pick up any review so this lets everyone know that a PR is waiting.
- Some additional questions about the impact on internal services, infrastructure and documentation are also templated to ensure that the full impact of the PR is discussed. There may be a need for infrastructure changes or for the support team to update documentation. Depending on the scope of the review, we also require confirmation that the PR has been tested on the range of browsers and platforms we support and the keyboard controls have been included (if appropriate).
The template is set up to provide a self contained environment for testing this change and deal with anything that might be touched by it. We link Github to our build system in Travis so that the reviewer can easily see that tests have passed and verify the output. The idea is that once the review is completed, it can be merged into master and deployed with minimal risk.
Conducting the review
First, test the code! The PR template will have detailed instructions for testing all parts of the implementation. The reviewer should follow those steps and ensure that everything is tested with good data, bad data and any edge cases. Trying to break it is good!
Sometimes, pre-existing bugs are found. These are tested against master as well and if found to exist in both, a note added to the review with a link to the existing JIRA case (or a new one opened). It might be easy to fix as part of this PR but is not always necessary. We usually don’t want PRs to experience scope creep.
PRs for frontend code will also include ensuring that everything is working from a visual perspective. This starts with checking the UI looks right but also includes verifying the implementation against designs and may involve the designers themselves.
The backend team may have additional considerations around backwards compatibility and what impact merging this PR might have on existing data and APIs.
Side effects of PRs can lurk in corners so it is important to test key parts of the application to ensure no unexpected regressions have been introduced. Parts of the UI which use the same CSS classes or React components are obvious places to start but there may be other cascading effects. Our test suite helps with this because we have extensive unit and integration tests, but there are always areas without coverage or which can’t be fully tested.
Unit tests can really help to understand why and how the code is working. A useful approach can be to intentionally introduce bugs into the code to see how the tests behave. For newer parts of the codebase, tests can often serve as documentation and so the reviewer should include a note about this if the tests are key to understanding the code.
For large changes, the test results should be noted in the PR comments. This provides a useful audit log of what was successfully tested and helps provide a methodology for commenting on changes with overall impressions, screenshots/gifs and reproduce steps for bugs.
Once functional testing is completed, the reviewer will add a comment to allow the author to start working through the feedback. However, the job of the reviewer isn’t over yet. The next step is to read the code, trying to understand every change at least file by file if not line by line. A good commit history will be helpful here because it allows the reviewer to understand how the changes progressed.
Having two separate steps – one functional, another a close code review – allows for a fully comprehensive review. That is why a comment is added to state the first part of the review is completed but that there are still more comments to come.
When something isn’t understood then the right approach is to ask questions. Equally, opinions are always welcome. The reviewer should think through whether the changes are logical and how the approach matches the rest of the codebase. If the reviewer finds changes they disagree with in style or strategy rather than function, this should raise a question or suggestion for an alternative approach.
Be specific in how you suggest alternatives. Saying something like “Why not just use the helper here?” Is not as useful as stepping through the exact changes you’d like to see. It not only helps the author know exactly what the reviewer is suggesting (no crossed wires) but it helps avoid code fatigue. After all, the author has probably been working with this code for a long time already!
There is a balance between disagreeing because it’s not how you would do something vs disagreeing because the approach is questionable. It’s important to remain open minded about methods you haven’t considered.
Long term maintainability of the codebase is important. If there’s something puzzling you now, it will be even more puzzling in a few months! Pointing out small things which could make reading the code easier will help with this.
Finally, the reviewer should summarise what they like, highlight the most important in-line questions and state that the review is done. It then heads back to the author to respond.
The challenge with written communication is transmitting intention and avoiding misinterpretation. Whether it is email, chat or discussion threads, it is all too easy to apply meaning which wasn’t intended. This can result in negative feelings and unnecessary conflict.
To help reduce the chances of this, there are a few style guidelines we try to apply.
For the reviewer
- Always assume the PR is the best possible approach within the constraints of time and existing code. Also assume the author has thought about it longer than you. The PR doesn’t necessarily show the full journey, only the end result.
- Try to be as positive as possible in giving feedback. There are reasons for the way the code is written the way it is but it is important to have an opinion and consider how the approach might be improved. The focus should be on that improvement rather than any negativity.
- Take care not to seem passive aggressive when trying to avoid negativity. The author is placing themselves and the code they have worked hard on in a vulnerable position. At Server Density we have many nationalities represented, all with their own styles of communication, often with English not being the first language. Be gentle and mindful of cultural differences when working with diverse teams.
- The sandwich method of feedback is useful to remind you to specifically highlight what you like at the same time as giving constructive feedback. Don’t be afraid to mention all the problems you see with the changes – this is the purpose of the review.
- Always write in full sentences. Using a few emojis can help but avoid jokes, irony and sarcasm – these are too easy to misunderstand in written communication.
For the author
- Assume the reviewer means well and there is nothing deliberately passive aggressive.
- Have an opinion too! You had reasons for implementing the code the way you did so don’t worry about explaining them. We often refer to PRs in the future because they contain important context about architectural and implementation decisions. However, the foundational design and planning work will have been done well before the PR stage and so re-hashing old architectural topics and decisions is probably not a good idea unless they are fundamental to the review.
- If you think the reviewer is missing a point, help them understand but try to avoid being too defensive.
- The PR is not a chat – that’s what Slack is for, and we don’t use Slack to conduct reviews. Think through the question and make sure you understand all aspects, then take time to answer.
- Accept feedback. If you find yourself arguing (even if just inside your head) about every comment then take a break. If you’re really unhappy then arrange a video chat (we use Hangouts) because the limitations of written communication can make it worse.
For junior team members
You will do reviews where the author is your manager – they may be the Tech Lead, Engineering Manager or even, occasionally, the CEO. They might know more about the code base and/or have more development experience. When you first join the company, we will try to only give you PRs that you can handle but the ultimate goal is that everyone participates in reviews, merges to master, and presses the deploy button. Don’t worry about who the author is – the above approach applies to everyone equally.
It’s a tricky balance to be thorough in your reviews at the same time as being speedy but you will get better at this over time. Conducting reviews is a great way to learn the codebase and is a key part of our onboarding process for new hires.
If you don’t understand something then this is a problem with the PR, not you. Don’t be afraid to ask. There may be some implied knowledge that was missed due to the assumptions of the author, so this is a good opportunity for them to teach you and improve our documentation! Highlight this and a video discussion will usually be the next step.
The most exciting bit, and probably the most nerve-wracking moment – merging and deploying!
Only commit and merge what you’re happy to maintain for a long time to come. Depending on the type of code this could be months or years, depending on what part of the code base it is touching.
Once a piece of code is merged, it is part of the collective work of the team. Forget who wrote it and treat it as your responsibility during the deploy process. All parts of the codebase are owned by everyone.
Unit tests, integration tests, functional testing, code reviews, PRs, automated deployment, staging, monitoring and rapid rollback are all there to ensure that you can deploy regularly, with confidence.
Thanks to Daniele De Matteis, Kerry Gallagher, Enrique J. Hernández, Sonja Krause-Harder and Richard Powell for helping with this post.