I have thought about this question a lot. I think the ultimate answer is "non-functional requirements": simplicity, maintainability, security, performance, test coverage (not the number; how many of the functional requirements are encoded into tests).
This comes from the point of code review: to make the software we are working on more useful.
There is a big list of non-functional requirements on Wikipedia; I will cover the ones I think are most important below. I'm biased toward maintainability because I've spent many years maintaining other people's code.
Simplicity
For me, this is the most important thing. Software dies when it becomes too complex. Development slows to a crawl when it is not possible to keep the important architectural components in your head. Changing anything becomes a process of trial-and-error which takes forever, and usually makes things even more complicated.
So, it is important to be constantly vigilant about simplicity. Sometimes that means saying no to a new requirement. Sometimes it means using a library instead of writing new code. Sometimes it means NOT using a library to avoid the complexity of having to manage that dependency. Often, none of the above are possible and the best you can do is refactor.
Simplicity applies at all levels. Requirements, architecture, module-level design, and individual lines of code. Even the size of an individual pull request. Keep them all as simple as possible. Jargon often used on this topic is "domain boundaries" or "high-cohesion/low-coupling" or "cyclomatic complexity" or "cognitive complexity".
Red flags for simplicity are:
- singletons/globals/side-effect-heavy code
- long, generic names
- many lines of code in one function
- unnecessary abstraction (also known as "overengineering")
- "surprises" or "magic" in the code
- Flag arguments (boolean parameters that switch behaviour)
- Inheritance heirarchies
- Mutexes
"Keep it simple" is my #1 thing for code reviews.
Test Coverage
When looking at test coverage, I will try to work out how many of the edge cases in the requirements for the change have been encoded into tests. For example, if the change is to send an email when an order is complete, there should be a happy-path test to make sure that whatever email interface is getting hit, and that the request has some of the strings the email is expected to contain. If the change is to process multi-leg orders, the tests should demonstrate how the obvious two orders get processed, but also that mixing a buy and sell leg is OK and that having three buy legs works, and maybe even that a leg with zero quantity or negative price is handled correctly.
Having tests like this makes the software maintainable. A new maintainer can come along and re-discover all the requirements that have been built into the program by reading the tests. There is no easier way for a programmer to learn the requirements (apart from having been there when the requirements were determined).
This allows new maintainers to understand the program -- and this allows them to keep their changes simple.
For a discussion of the other kind of tests ("don't test the glue"), see
https://stackoverflow.com/q/12860657. Aiming for 95% test coverage (or whatever) almost always leads to writing useless tests that become a maintenance burden.
Readability
Going hand-in-hand with reading the tests, maintainers will spend a lot of time trying to figure out the code itself. There are a few things that help a lot here.
- Comments that explain WHY. How, what, etc. I can get from reading the code. Hopefully, the imporant part for understanding -- the why -- comes from reading the tests; sometimes it doesn't.
- Lots of descriptive names -- variables, functions, etc. Don't use generic words ("-Manager" is my pet hate), they don't help. Be specific.
- Auto-formatters. All the big languages have this now. Stop wasting your time by not using them. Black, prettier, clang-format, go-fmt, whatever.
Teamwork
The best code review is one you do together. It's not worth trying to have difficult conversations through text when you can talk to someone directly.
If you're doing a code review and find something that's not a super-quick fix, it's almost always better to discuss it in person.
For everyone else's benefit, it is generous to record the outcome of the discussion in the review.
