Feedback is critical in any engineering organization – and that feedback often comes through code reviews. Junior engineers learn how to manage complexity, simplify the logic, and develop the codebase from senior engineers. But on the other hand, even the most senior engineers benefit from having a second pair of eyes on their code.
Yet, very few organizations set standards around their code reviews. Using a checklist can increase code quality across the entire organization. In addition, it serves as an excellent onboarding document to train new reviewers, expanding the pool of reviewers and expediting the review pipeline.
I've compiled a starting point of 10 questions to ask when reviewing code. This checklist is available in a copyable Notion template here.
What do you look for in a code review?
— Matt Rickard (@mattrickard) March 17, 2021
Logic
- Does it make sense?
- Can it be done simpler?
After correctness, simplicity might be the next essential item on the checklist. Simple is hard. Simplicity doesn't always mean the fewest lines of code, either.
Complexity
- Can we maintain this?
- Does it add new dependencies?
Code needs to be looked at through a net present value lens, accounting for the costs of maintaining this code in the future.
When code adds new dependencies, those dependencies should be checked for necessity (do we need this?), security (who wrote this?), and usage (are we using it right?). Internal dependencies should make sense and not cause circular dependency or diamond dependency headaches.
Tests
- Does it include unit tests?
- What test cases are covered?
100% coverage is rarely a goal but look for tests covering critical and non-trivial code paths. Tests that don't test anything become technical debt.
Documentation
- Is the documentation correct?
- Can others understand it?
Checking for outdated documentation is just as important as adding new lines. It should be easy to understand by the target audience – engineers on other teams or, in some cases, non-technical coworkers.
Style
- Naming
- Consistency
Style will differ for most organizations, but common threads are naming and consistency. Naming should be descriptive but not overly verbose. When there are multiple correct ways to do things, favor consistency with existing code.
Likely, you'll want to add your own items to each category specific to your organization: commit message style, documentation expectations, and more specific language-level idiosyncrasies that can't be automated in a formatter. As for guidelines, I'd say to keep the checklist short and simple. Unnecessary verbosity will cause developers to abandon a checklist quickly.
Other ideas? Tweet them at @mattrickard