Code reviews are an integral part of our development process as they serve to ensure two key goals: the learning and growth of team members and the delivery of healthy code. While we use various tooling to help us programmatically catch errors and formatting preferences, they are by no means a replacement for extra pairs of eyes.
The code review process should be collaborative and inclusive for the author and reviewers as well as any other teams members wishing to understand the scope of the changes that were made. Most importantly, code reviews are not about being right or wrong but should focus on knowledge transfer and ensuring that requirements are met.
As on author, the most important rule is to be respectful of the reviewers' time. The easier it is for someone else to read your PR, the quicker and more likely that PR is to be reviewed. While not all PRs are equal, where possible, consider the following to improve the changes you submit:
So, the "big" question... Q: How big should your pull request be? A: Small.
Research shows that no matter how large your pull requests are, reviewers mentally tap out after 1 hour. And, a review of 200 - 400 LOC in 60 - 90 minutes typically yields 70 - 90% defect discovery. Beyond 400 lines:
- the rate of defect introduction increases
- the rate of defect discovery by reviewers drops significantly
- reviewers are much less likely to start the review process quickly, or at all
Large PRs may also block other developers who are depending on a feature or bug fix to be deployed.
Aim to keep your PRs to 400 LOC or less. If your PR must be larger, try to find logical break points where it can be split up, and create several PRs that can be reviewed independently.
Identify and create digestible changes via several commits. This allows your reviewers to follow the progression of your implementation and helps to provide breakpoints from which to drop off and pick back up in a segmented review. Breaking up your changes also allows, if necessary, reverting specific changes more easily based on feedback you may receive.
Use meaningful commit messages so your changes are clear and have proper context.
Don't forget to squash your changes before merging after your PR has been approved.
Review your own code before submitting it to others for review. Ensure that all your changes meet any existing documented guidelines and have been properly tested. Debugging statements as well as commented out code should be removed.
Provide as much context on the scope of the story and how your implementation achieves that. Add screenshots/videos to show your changes in action. This adds visual context and helps reviewers to identify edge cases. For changes affecting UI components, publish a Storybook example and include a link to it in your PR description.
npm run storybook:publish -- (tagname)
If you've used an implementation approach that's different and may not be immediately obvious, add comments in the PR to help save reviewers time in understanding what was done and why.
Include step-by-step instructions on how to demo your implementation. This drastically reduces the set up time for the reviewer and also helps as one way to identify if there are cases that you forgot to consider.
The code review process should be collaborative and engaging. Address or answer all questions posed in comments.
Be mindful that it does take time and effort for reviewers to properly review your code. If they leave comments, show your understanding and appreciation of the feedback, even if it's a simple emoji :)
As a reviewer, your goal is to help maintain the health of the codebase as well as to help other developers grow and learn. While this burden is high and requires endless patience, with practice, it becomes easier. Remember that code review is a collaborative process and the right approach to giving feedback impacts the dynamic of the conversation.
Perhaps the most important consideration when performing reviews is being empathic. Communicating in a polite manner and expressing understanding helps to soften conversations and supports engagement. Try to provide useful critique not criticism and use it as an opportunity to bring up your peers.
Language goes a long way when setting the tone of the conversation. Comments with a demanding tone create a higher risk of transforming into a heated argument or making others resentful. Maintaining a gentle tone tends to settle most discussions before they escalate and also helps to reduces misinterpreting feedback as criticism or as a personal attack.
Use emojis :)
One of the best ways to articulate the changes you suggest is by drafting code examples. Be it an inline change, an entire block or a function, code examples provide a clear picture for the author for what is expected and how it improves the existing implementation.
Code examples are also quite helpful to colleagues who may not be as familiar with a concept you're referring to. Where possible, reference links that support your example to provide a more complete understanding.
Part of having a healthy codebase means having predictable, consistent styling across the project. Ideally, all code should look like it was written by one person. So, we have a number of tools: eslint, prettier, stylelint and our code style guides to formalize those expectations. When we stumble across code that does not conform to these guidelines, drop a polite comment which refers to the line that does not meet our standards.
Arguing about style, formatting or best practices is waste of time in code reviews. If there has not yet been a formal convention defined for your suggestion, defer the thought for a discussion with the team, then formalize and document it in the appropriate style guide. PRs should not be blocked because of new or undocumented standards.
When providing feedback, leverage engineering concepts in explaining the reason for the changes you are suggesting. The best approach comes from a comment that doesn't have the author asking "Why?".
Software development is both an art and a science and there is not always one right answer to every problem. Nevertheless, maintain objective feedback such as:
I found this hard to understand. How about we restructure this block considering xyz principle...
and avoid making value judgements like:
This is messy and confusing. Use xyz structure instead.
Most reviews tend to focus on what went wrong or suggestions for improvement. Try to also look for genuine opportunities to praise the positives such as the implementation of an elegant solution or useful refactoring/clean up. This encouragement helps to reinforce positive behaviours and motivates continued effort.
Whether you're the author or a reviewer there are common behaviours we should all practice in order to maintain a healthy code review process:
- Pass off opinion as fact
- Ask judgemental questions
- Use sarcasm
- Use negative emojis
- Use questions or recommendations to drive dialog
- Take long-winded discussions offline, then provide a summary online
- Collaborate, don't back-seat drive
- Address and/or reply to every comment
- Use opportunities to teach, and don't show off