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:
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?”
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.
- 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?
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.
# 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.
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.
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.
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.