Code reviews are an essential part of software development. To be good at the craft, we must be able to review the craftsmanship of others. The more books you read, the better you write; the more code you review, the better you code.
In this post, I’ll be sharing a 12-item code review checklist. This is a breadth-first traversal; each item is presented at an extremely high-level and deserves more of your attention.
#1—Does the code work?
- Take your time and understand the implementation. This is a basic pre-requisite before you review any code.
- Don’t assume the code works out of the box!
- If you think you found a bug, there’s probably a bug.
#2—Are there any abuses of memory?
- If appropriate, does the code release all the memory it originally allocated?
- Does any operation potentially use a dangerous amount of memory? Unbounded DB reads, mis-use of data structures, etc.
- Is the code too aggressive with local caching?
- Large usages of memory should be called-out and justified by the author.
#3—Are there any abuses of performance?
- Would these changes disrupt the normal performance of the application?
- Extra I/O that increase the response latency of a web application.
- Extra logging that slows down the runtime of a software simulation.
- For web applications, can any business logic be pushed down into the DB via better queries?
#4—Are there any local concurrency issues?
- For multi-threaded applications, are the code changes thread safe?
- Are multiple threads using non-thread-safe data structures?
- Are there any potential deadlocks?
#5—Are there any distributed concurrency issues?
- Is the application you’re reviewing part of a distributed system? Most likely!
- If the application connects to a SQL DB, are the correct transaction isolation levels set up to ensure proper application behavior?
- If the code orchestrates calls to various other services, are there any dangerous assumptions on the state of distributed data?
- In general, does the code access and use shared resources (databases, caches) properly?
#6—How risky is the change?
- If the change is risky, has the author added defensive measures to make sure something terrible doesn’t happen?
- Has the author accounted for monitoring and observability?
- If the code lies in a critical path, does it have to be?
- Have disaster scenarios been thought through? Downtime, corrupted data, lost money.
#7—Should the code review be broken up into smaller pieces?
- Is the code review attempting to handle too many concerns? If so, ask the author to split it up.
- Do the changes tell a coherent story? Beginners struggle with story-telling through code. A story behind your changes is crucial.
- Changes must be deployed logically, correctly, and safely.
- Allows other people to understand what you’re doing.
- If either of these conditions are not met, the code shouldn’t be approved.
#8—Does the code follow existing patterns?
- Does the code go against existing patterns?
- Are stylistic standards maintained? Indentation, line length, comment style. This is easily solved with an automation.
- Do the changes go against established coding patterns? Exception handling, interface design, testing philosophy.
- If the author is going against existing patterns, and doesn’t seem to have a solid reason—say something.
#9—Does the code introduce new patterns?
- New features bring new code. Inevitably, new patterns will be introduced.
- New is not bad.
- If the author has introduced a new pattern, does it make sense for what he/she is trying to solve?
#10—Can any language feature be utilized better?
- Is there something you know about the language that the author doesn’t?
- Are there any language constructs that aren’t utilized properly?
- Are there any locations where a language construct would make the implementation cleaner?
#11—Can any framework feature be utilized better?
- Exactly the same as above, except for frameworks. React. Rails. Spring. Django.
- Are there any framework constructs that aren’t utilized properly?
- Are there any locations where a framework construct would make the implementation cleaner?
#12—Is the code readable?
- At the end of the day, code is communication.
- Your teammates need to read it. You need to read it.
- Don’t be afraid to call out readability issues. One-character variable names, deeply nested conditional flows, huge functions, huge files.
- Is there a good balance between abstraction and concreteness? Tons of abstraction may cause unnecessary amounts of mental energy.
Bonus: Other Things To Keep In Mind
- Not all of these points are relevant for every code review.
- For larger code reviews, you may have to review it, come back to it, review it again—multiple times over.
- Code reviews are a good place to learn and ask questions.
- When commenting, be clear about what needs to be addressed now versus what can happen in a follow-up.
- Be reasonably prompt; don’t take two weeks to review your teammate’s code.
- Don’t be a jerk.