Code reviews are a necessary part of our roles as developers. We are not only responsible for contributing our own code, but also to inspect the code of our peers. The practice of code reviews provides many opportunities: checking for errors, knowledge sharing, and collaboration. To ensure we are making the most of our code review process, we can set some expectations for code reviews and how to approach them depending on your perspective; as the writer, as the reviewer, and as a team.
For the writer
Annotate your own code
Be the first reviewer! Make use of the pull request (PR) comments and PR template (if one exists) to be descriptive of the changes you are making and why. This will ease the barrier to entry for someone reviewing, and allow them to be more efficient when it comes to their reviews. Sometimes there is context that might not feel appropriate to include in code comments, but should be brought up in documentation/PR review comments. The reviewer most likely will not have all that context, and this is the best place to provide it.
Make pull requests reasonable size
If we can split work into multiple parts, we should to ease the burden on the reviewer. Use different appropriate techniques to break up your PR: make use of feature flags, break out the static pieces of the work (the models, the interfaces, setup, etc), and implement the business logic separately. Build the pieces first, and then do the connecting or "activation" at the end. Your reviews will happen more quickly, regularly, and thoroughly the smaller they are.
Effective selection of reviewers
Ask yourself: “Who needs to review this change?” Consider your confidence in the work, and if the changes are to critical path code. If you are low confidence, or the code changed is critical, and you know which of your peers is most knowledgeable in the area you may want to include them as a reviewer. If you are really confident in the change, you may only want to include your immediate team members as reviewers.
Own your pull request
It is natural to see a PR creation as a “final step” to completing a change, and we sometimes can get in the head space that there is minimal to no work left. Don’t stop yet! Continue to be active and see the PR through to merge. Respond to comments critically, and poke for reviews when necessary. Always keep in mind your WIP (work in progress) and avoid overburdening yourself because you underestimated the feedback. And even when you have merged your PR, consider any follow-up that may be necessary, like messaging the team to inform them of new patterns, practices, or upgrades that have a global effect on the code base.
Don’t take it personally
When we receive feedback or comments on our pull requests, we can reflexively get defensive about our work. Take a step back and think critically about the comments: Is the reviewer correct? Could the code be clearer? Assuming the reviewer left a review with appropriate tone, approach the feedback from an objective perspective, instead of an emotional one. Communicate clearly in your responses, and provide any additional information that the reviewer may be missing. If it would help, send a video or get on a call with the reviewer and cut down on the back and forth or wasted time to misunderstandings.
For the reviewer
Schedule reviews
Despite most of us valuing code reviews highly, and it being a required part of our day-to-day process, we can sometimes fall into inconsistent patterns when reviewing. Try to make reviews a habit, bring some regularity to your individual workflow and aim to spend a certain amount of time each day reviewing. Along with booking the time, be aware of the response time you have to comments on your feedback and try to be quick to respond. There are better outcomes with quicker responses as less urgency is built up over time so we can spend more time discussing the work, instead of the external forces that may be pushing us.
Be precise
Be precise, in language and intent. Use clear indicators of blocking and non-blocking comments. It can be hard to communicate asynchronously via PR comments, so we should try to be as explicit and clear when writing them as we can be to help the PR creator understand our perspective. A very useful tool to have in your repertoire are annotations. Use annotations like below in your comments to give the user some immediate context:
nit- Indicating “nit-picks”, usually small improvements or suggestions which most of the time are non-blockingsuggestion- Indicating an improvement or addition that can be madeissue- Indicating a possible issue with the implementation, usually a blocker to approvalpraise- Showing appreciation for the improvements being madechore- Indicating follow-up work that may need or want to be done after the current PRtypo- Indicating a typo in the code or comments around it
You can even take it a step further by adding some labels to your annotations like: nit (blocking), for example: nit (non-blocking): I think we can use a ternary instead of an if else block here.
What to look for
As the reviewer, we cannot possibly catch every issue with a PR, and we should not aim for that. Instead we should focus our efforts on a few important aspects of the PR:
- Functionality - Does the code do what the writer intended? Is the intention appropriate for the area of code changed? Have edge cases been considered?
- Complexity - Is the solution more complex than it needs to be? Can I easily comprehend and grok the code? Is the code attempting to fix problems we do not have yet?
- Tests - Does the code have automated (unit, integration, e2e) tests added? Are there any tests missing? Are the added tests sensible, and testing the correct things?
- Clarity - Is the code appropriately commented? Is the code style appropriate? Are there unanswered questions or unclear purpose to parts of the change?
- Documentation - Beyond code comments, should there be documentation updated or written along with this change? Does the change make parts of the readme outdated? Does the code deprecate features that are documented elsewhere?
Tone of review
We should always aim to be courteous and respectful to the writer whose code we are reviewing. Be kind, clear, and helpful in your approach. Be objective and write your comments in response to the code, and not the person that wrote it. Explain your reasoning, but be open to new approaches and to learning something yourself. Avoid giving explicit direction and instead encourage the writer to think for themselves and make decisions by phrasing your comments in the form of a question, pointing out other options, or opportunities for simplification.
For the team
Be proactive
We usually do code reviews asynchronously for convenience, but sometimes it is best to leave the keyboard and talk it out. If you are finding you are leaving many comments on a PR, or you are having trouble understanding the code, talk it out with the writer. Either get on a call, in person, or make a video to avoid getting lost in ever growing chains of comments. A lot of time and frustration can be saved by hashing out misunderstandings face-to-face.
Encourage good relationships
Above we have discussed a lot of aspects of each side of the code review, but we should always remember that code reviews are a collaborative process. They are opportunities for all of us to grow and learn. The code that you are writing or reviewing will be owned and maintained by the entire team. Invest in the relationships with your peers by being respectful, considerate, and courteous and they will be more likely to reciprocate. Such investment will build accountability and trust not only with each other, but the work that we do, which in turn will make you a more effective and performant team that releases high quality products, and does so quickly and collaboratively.
Was this helpful?
