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 to 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. By using a checklist, you can increase code quality across the entire organization. Better yet, 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.
- Does it make sense?
- Can it be done simpler?
After correctness, simplicity might be the next essential item in the checklist. Simple is hard. Simplicity doesn't always mean the fewest lines of code, either.
- Can we maintain this?
- Does it add new dependencies?
Code needs to be looked at through a net present value lens – that is, account 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.
- Does it include unit tests?
- What test cases are covered?
100% coverage is rarely a goal, but look out for tests that cover critical and non-trivial code paths. Tests that don't test anything become technical debt.
- 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.
This 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 quickly abandon a checklist.
Other ideas? Tweet them at @mattrickard