Code review is the most effective teaching tool for onboarding team members, upskilling newer developers, and becoming an expert on the product and technology.
But it isn’t effective if you breeze through it.
Code Review should take a good amount of time, longer than you’re probably comfortable with. The reviewer should understand what is being changed, why, and how it was resolved. Once these are understood, the reviewer can effectively critique the work.
On my teams, we prioritize quality over speed. Ironically, we often end up delivering faster than teams that prioritize speed — and with significantly fewer bugs.
Code Review takes a significant amount of time invested up front, but it creates a shared knowledge that eliminates tons of basic mistakes down the line. Ultimately this saves tons of time and effort for everyone.
Here’s my code review checklist for these teams:
✅ Understand the original problem
Whether a feature, a bug, or an enhancement, understand WHY we are doing this work in the first place.
- Review the work’s ticket (if there is one) and understand the requirements
- Talk to the developer or the reporter if further clarification is needed
Answer the question: “What’s the problem that this work solves?”
✅ Locally replicate the problem
git pull develop
npm start
Take the branch that the PR branched from, and physically watch the problem occurring.
When you can see what the problem is, you can diagnose the proper solution more effectively.
✅ Understand the required fix or feature
- Based on what you’ve seen so far, what would you determine are the possible solutions? Try to think of a few options.
- What areas of the code might need work to solve this problem?
- From a UX perspective, is the solution that was proposed in the requirements actually the best way to fix the issue?
✅ Review the PR directly (understanding dev’s approach, checking syntax errors)
Now we start looking at code. Review the code’s diff in your repository (Github, Bitbucket, etc). This is the easiest way to see the “before and after” of what’s been changed, and the major pieces of the work.
- Is the overall approach a good one? Is it adding too much complexity? Is it working on the right area of the code?
- If so, are there any basic mistakes? Syntax errors, poorly named or mistyped variables, extraneous comments, formatting issues, or typos?
- MOST IMPORTANTLY: Do you understand everything about how this code works? If there’s something you don’t understand, you should add a comment asking for clarification! This will either a) help the developer teach you new tools, or b) help them realize that their code isn’t clear and needs to be cleaner.
✅ Locally play with the PR branch
# on root branch
git pull
git checkout the-pr-branch
npm start
- Pull down the code and see the solution in action, explore how pieces are being linked together and how interactions work in the code.
- Try to break the feature again.
- Change some code and see how it changes things.
- See if there are any lingering errors in the console or IDE which need cleanup
These are all steps you will take to confirm that the code works as expected.
✅ Check if unit tests are testing every interaction effectively
Every codebase should have unit tests, and those tests should check that every function is working exactly as expected with both “usual” input and “unusual” input.
Are you actually testing everything you think you’re testing? Code coverage tools like Jest or SonarQube are easy to implement and can tell you if you are truly testing everything — I personally like a coverage level of 80%.
For each test:
- Check if the output is correct with good values. Check that function handles bad values gracefully.
- Check that all logic inside of a function is being tested effectively. Is something inside of a function hard to test? It probably needs to be pulled out into a separate function and tested separately.
- Check that all unit tests are passing and not throwing odd/unnecessary errors in the test suite.
- Check that your code coverage level has not fallen below the coverage threshold you configured. If it has, add more/better tests where the report identifies weak coverage.
✅ When you’ve given feedback, do it all again when changes are pushed
You have reviewed one PR, and it probably took a while. Good! But it doesn’t end here.
When the developer pushes new commits to address the feedback of you or others (fixes and improvements), you should go back and test it again. When small things are changed, it can have a big impact on other areas of the application. Run through the checklist again.
The code in a PR should always be considered “ready for production”, never accept “I can fix that later”. It should always be reviewed expecting a high quality of delivery.
🤔 Conclusion
This process takes a while and is intentionally intensive. That’s a good thing!
When you follow this process:
- Fewer bugs get through
- Developers understand more deeply about the product and learn it MUCH faster
- Reduces the amount of “re-reviews” because — when done effectively — devs learn from each others’ mistakes in other PRs. This dramatically speeds up time that PRs spend “in review”
- The team develops a culture of constant improvement and peer mentorship
Those are all ultimately goals of every software development team. So put better PR review practices in place, embrace the initial heavier load, and watch your team’s quality of output improve dramatically!
Paul is an engineering manager at Skookum, a former UI/UX designer, a snowboarder and golfer in Colorado, and apparently a dev blogger. Follow me on LinkedIn, the Tweeter, Github, or anywhere you can find deep snow.
Need help building a product or a team? I’ll chat with ya on LinkedIn.
Top comments (5)
Thanks Paul for sharing your experience!
You have a great process, in most companies where I have ever worked we were spending much less time - mostly doing code review in code review tools (Crucible, GitHub etc.), without pulling and running a code in a PR.
Could you please tell, if you think that having the best code reviewers for each PR based on their experience in related code components(parts, modules) is important?
In any case, could you please share your strategy for selecting code reviewers?
In a large-application, I think it’s definitely valuable to have subject matter experts review the code that’s being developed. The ideal scenario (read: not realistic but what we should strive to do) is that anyone who may touch this area of the application gets eyes on it, understands it, and approves of its implementation.
That said, selecting code reviewers is largely based on your team structure and application organization. You don’t need back end teams reviewing front end code for example. But if there’s someone who even might touch this feature later, they should probably review it, even briefly.
In all of my projects, we enforce a minimum number of approvals before a PR can be merged (usually 2-3). This way we can be sure a few others have seen, understood, and quality-checked the code.
Sorry for my delay in answering!
Have you tried to use tools like mention-bot, which suggests best reviewers based on commits history?
Or do you use CODEOWNERS or any other things to help with selection of reviewers?
I have not, those sound interesting! I have always organized my teams in either application specific or feature specific groups. This generally makes picking the reviewers easy — pick those people, plus any technical manager or architect who may be involved in overseeing the whole production across teams.
Yes, feature teams is a one way to solve the problem. However, as a developer, I could say, that sticking the whole team to a fixed set of features often leads to lower retention rate.
If in other case, the developers are not sticked to a sed of features, then there could be a mess with responsibility: "who owns XXX" question start appearing often in chats.
As mention-bot is dead and CODEOWNERS is static, I decided to create my own automatic recommendation tool.
Feel free to contact me in Linkedin if interested.