Observations on things I have looked for in a code review

This is from an internal email I shared with the team a year ago. Over the past several years of looking at pull requests (PRs) and doing peer reviews of my and others’ code, I have observed that there is a pattern I typically follow.

There are 2 broad categories of things, all of which assume that the person reviewing the code knows enough about the product, the tech stack and the systems involved.

Quantitative

These are things that can be measured or validated against something objective (a number as in code coverage, yes/no answer, clearly defined best practices in the industry, team agreed coding conventions, etc.).

• Is the code doing what it’s supposed to do? — this is easily verifiable by looking at the ticket in your ticket management tool. Make sure you understand the requirement and the acceptance criteria clearly before looking at the code.
• Is it tested? — though there is some subjectivity in terms of unit or integration tests, mock vs real data testing, you could get a “level of comfort” testing by looking at the tests. Always look for tests first before looking at the code.
• NFR (non-functional requirements) — does it perform well, is it secure, is there enough logging and monitoring? There are many obvious pitfalls you can look for — nested loops, unnecessary iteration, too many IO calls, queries without where clauses or improper joins — all of which can be checked with the naked eye without running the code. If you have automated tests for performance, you are probably in the top 1% of the good companies.
• Is it doing one thing and one thing only? — there are times where people sneak in 2 minor bugs in one PR because it’s convenient. In a small enough code base and a small enough team, this might be okay-ish. There are several inconveniences in doing it — increases the cognitive overload of the reviewer, makes it harder for automating ticket closures, one deployment has two things to test for and if one test passes and the other has issues, we will have to revisit the merged PR and look for separate commits (hopefully they are covered in 2 commits).

Qualitative

These are things that are subjective and have no clear right/wrong.

• Do we really need to do it this way? Is there any other way we can achieve the user’s needs and preferably without writing code — by design, by eliminating the interaction that could result in an error, pre-validation of a form input?
• Is the code/design consistent with what we have done so far or agreed earlier? Naming conventions, design patterns, eslint-like rules, amount of testing, kind of testing, documentation?
• Is this maintainable? Part of maintainability is readability and other is extensibility.
• Is it simple enough that people will understand it without someone explaining it? Organisation of code, easy to understand variable names, simplified logic, no black magic code or no “don’t change this” logic.
• Can this be improvised iteratively or is it trying to get it right the first time around? This is hard since you only look at the PR after it is submitted, at this point the developer already might have spent a long time. This is helpful in a peer-programming exercise.
• Can this code be abused by another developer to do what it’s not envisioned to do? Did we make it simple enough that it does only one thing? If we overload in the first instance, someone will overload it again in a few weeks and before long, you got yourself a monstrosity that is hard to even refactor.

In the end, I believe in two guiding principles(beyond what is generally accepted as good code):

• Best code is no code at all — https://blog.codinghorror.com/the-best-code-is-no-code-at-all/ — Jeff Atwood
• The complexity of a complex piece of code increases exponentially over time — this might probably be me saying. Every time I see a really complex piece of code, several months later, there is so much happening around it, that it becomes insanely hard to debug/refactor it.

Obligatory disclaimer: As with any generalisation, these are subjective and my perspective. There may be circumstances where I might throw these out of the window and get something merged. But that’s for a different conversation.