10 Commandments of Successful Code Reviews

A checklist for more productive code reviews

Photo by @charlesdeluvio on Unsplash

Code review is a process that you should often do. These can be done in four different contexts:

• As a team lead/tech lead/scrum master, reviewing code before it can be pushed to production
• In the context of an open source project where one should review code opened in a Pull Request (PR)
• In a classroom. A teacher reviews their students’ code assignments
• By a fellow engineer helping out to better both the reviewer and the reviewed individual

During my years of reviewing lots of code in these different contexts, I accumulated some guidelines I use myself. With my team, we recently went through the process of what makes a code review (CR) successful and productive.

Here are some things we compiled together that I thought I’d share here.

Code Quality

What’s code quality? In my opinion, it consists of three main parts: style, readability, and design.

Style — the entire code base should look like it was written by a single individual and follow the project's style guide. This is also the easiest to uphold.

Tools like linters and formatters automagically make this part so easy, and if they can be used, you should. For instance, pre-commit hooks are a great idea.

Humans tend to be more open to automated processes telling them how to fix their code (or even better — fix their code for them) than someone pointing out their mistakes.

That being said, you shouldn’t rely on them to do the work, and you should make it a habit to follow the style guide even if they don’t exist, as not all projects have them. (It’s like still using your rear-view mirror even though you have that rear-view camera in your car when you park. You don’t know when it will cease to function or when you’ll drive a car that doesn’t have one.)

Readability — this includes variables, functions, classes and method names, inline comments, doc-strings, etc.

Think of it this way, the next programmer after you should be able to maintain and understand your code six months after you wrote it. It’s bad enough you won’t really remember what you were trying to do. Think how annoying it will be for the next one to decipher what you meant to do!

(Our take on it, which is quite known, is that you should assume this next programmer trying to understand your code is a psycho-killer, sleep-deprived, annoyed individual trying to fix your code in the middle of the night. Oh, and they know where you live!)

While some believe the code should be self-explanatory as much as possible and comments should be rare, I disagree with this notion, as writing comments makes your thinking process obvious. Especially in cases where the “obvious way” was not used, so why did you choose this special other way? What was the reasoning? You obviously tried something that didn’t work, so you had to figure out a workaround.

It would be easier to maintain if the next developer understands your reasoning and doesn’t get stuck on the same things when they’ll need to change something.

That being said, the approach of “self-documenting code” still holds. Variable and function names should “declare” what they are doing or what functionality they serve.

Design — this one is the hardest as there are different approaches and ways to do the same thing, but this is where one shows how good a professional they are. Just keep learning and bettering yourself.

Frankly speaking, this is the most crucial part of a code review, which should be the part you focus most of your energy on.

I’d like to borrow from the Zen of Python (and similar texts exist for other languages as well). I reordered it here for context, but you can just import this in a python terminal and see how it looks for yourself.

I marked each row with either (S) for “style,” (R) for “readability,” and/or (D) for “design.”

(S) Beautiful is better than ugly.

(S/R) Readability counts.

(R) Explicit is better than implicit.
(R) Simple is better than complex.
(R) Complex is better than complicated.
(R) Flat is better than nested.
(R) Sparse is better than dense.

(R /D) Errors should never pass silently.
(R/D) Unless explicitly silenced.
(R/D) In the face of ambiguity, refuse the temptation to guess.

(D) Special cases aren’t special enough to break the rules.
(D) Although practicality beats purity.
(D) If the implementation is hard to explain, it’s a bad idea.
(D) If the implementation is easy to explain, it may be a good idea.
(D) There should be one — and preferably only one — obvious way to do it.

The Ten Code Review Commandments

Well, it’s not specifically commandments, more like things to notice and go through.

0. Review Your Code Before Submitting

Before anything else, realize that it’s on YOU as the code's author to ensure that everything is tested, that you reviewed the changes you made, and that they adhere to your project’s style guide, standards, and conventions.

Use the following bullet points to ensure you are ready to be reviewed.

1. Functionality

• Does the code do what it needs to do? Are all requirements fulfilled?
• Is there any existing functionality that is affected by these changes? How? Did any inconsistencies get introduced?
• Is everything working in terms of UI (any user interface, anything from CSS to CLI or API)? Did something break? Check all scenarios that are somehow adjacent to what you’ve done. Better yet, perform thorough tests on everything you can think of.
  Automated tests are great, and you should use them as much as possible, but tests are sometimes written so that they could pass (another problem on its own). See it in your own eyes for a second.
• Aspire to be “pixel-perfect” and/or “logic-perfect,” with all common and edge cases taken care of.

2. Readability

• Is it easy to understand what each piece of code does?
• Is the flow clear for a maintainer (better yet, for someone on an “on-call” duty?)
• Is the code formatted/linted? Does it follow conventions?
• camelCase? kebab-case? CapitalizedCase? snake_case?
• Are comments added at every turn where the code is not self-documenting?
• If one approach is more frequently used (either by us or as a convention), but we chose another, is the reason documented?
• Are variables properly named? Do they describe what they do? (variables named var paul, john, george, ringo while amusing, are not very useful)
• Is the code free from any obfuscation?

3. Design

• Are the DRY (Don’t Repeat Yourself) and KISS (Keep It Simple and Stupid) principles followed?
• Programming is essentially building a language out of another language where each function is a verb that might be used in the higher language (shout-out to the mental game of Python by Raymond Hettinger). Did you follow this principle?
• A function should only do ONE thing (if its name is doThisAndThat()it should probably be two different functions)
• Any side effects that are not obvious? (If a method changes the passed argument by-ref or changes the state of the class attribute without it being clear — it might not be a good idea as it is hard to follow unless explicitly noted)
• Separation of concerns followed?
• Organization — all components, classes, methods, and so on are in a suitable and obvious place. Will it be “where I’m looking for it”?
• No hard-coding! Use consts, settings, statuses, lookup tables.
• Did you use design patterns? Don’t reinvent the wheel. It’s easy to spot the “strategy,” “observer,” or “singleton” pattern (all mere examples). So when they are used, it’s easy to understand what is done. Don’t try to create your own version of them, as it might be confusing.
• Usage of standard libraries? Again, don’t reinvent the wheel. Don’t use your own sorting mechanism. The battery-included version would probably do the trick.
• Is there any unwanted/unneeded complexity?

4. Maintainability

• Comments, comments, comments.
  Any typos or unclear comments?
• Are we logging exceptions? Are any exceptions silenced? Why?
• Are unnecessary notifications triggered?
• Is user behavior monitored (where applicable)?
• Is the code testable?
• Were tests added/amended?
• Is the code configurable?
• Are commit messages clear and describe what was done in each?
  Bad examples:
  - Fix
  - Done
  
  Good example:
  - Added permissions to staff only for the object’s admin view.
• Are commits as atomic as possible?
• Are commit messages/branch names tied to an issue? Usually, having the branch name or the commit message has the issue reference number means it may be easier to understand the context in which the changes were made.

5. Performance and Scalability

• Any optimization for time and space complexity?
• Is the code scalable to at least two more orders of magnitudes? (If you expect only ten users, ensure it won’t break at 500. If it breaks for 50,000, that might be OK. While several orders of magnitude might be an unnecessary overhead to foresee, you don’t want it to break as soon as you hit ten more users)
• Does the code cover any failure scenarios?
• Was instrumentation used? Reporting, metrics, alerts, etc.
• Access to database/network/files — as little as possible (obviously, this cannot be zero, but don’t hit the database in a for loop. Use bulk inserts or something)
• Make sure you have as little reliance on the machine itself. (Machines fail and are disposable. In the age of cloud computing, a server can replace another at any point. Keeping your logs on the web server is a bad idea as they WILL disappear and be erased with the server when you need them the most)

6. Security

• Permissions used and enforced?
• Any implementation bugs that might be easily exploited?
• New dependencies — actively maintained and screened for known vulnerabilities? Any security-related open issues you should be aware of?
• Authentication used?
• Authorization?
• Input data validation?
• Parameters and user-provided data escaped, so they are safely used?
• SQL- and code- injections handled? If you use a raw-SQL query or DML for some reason, make sure the user-provided data is not applied as-is without proper handling. You don’t need to be a security expert for that. Just use best practices.

7. Miscellaneous

• Was everything tested with the end-user in mind?
  For example, loading states, errors, input validation, amount of data, empty states, edge cases, features on/off, unexpected error handling, etc.
• Are any feature flags necessary?
• Do we break anything in the integration? Do we rely on a certain way or order of deployment (database migrations, for instance? Or service dependability?)
• Any post-deployment requirements we should be aware of? Are they documented? Can it be automated?
• Do I feel confident that I can sleep at night once this is deployed?

Closing Thoughts

Like almost everything in life — practice makes perfect. The more code your review, the better you’ll get at it.

My background as a programming instructor led me to see so many different ways to solve the same problems that it became somewhat easy for me to review code rapidly, find caveats (or even bugs) and track down the problems and changes that should be made.

Since then, I became a team lead and a CTO (currently at Brew), which became a habit of mine, to keep reviewing some code before it gets to be part of our repositories.

I hope this checklist will help you when you need to prepare for a code review where you are the reviewer or the one being reviewed.