idea

Code review is an enforcement mechanism aiming at standardizing and raising quality by having a second person look at the code going into the repository.

Code reviews check for multiple things:

code cleanliness, notably smells^[3] which indicate logical or readability gaps^[1]
code is easy to read, will facilitate maintenance, will be understandable by a new joiner a year from now and when the entire team will be gone.
pre-defined criteria of testing, such as coverage, are met. In particular, that edge-cases are accounted for, and that potential regressions are backed by a corresponding validating test.
code does not have logical gaps. If some are found, then corresponding testing should be added
a better or simpler way could have been used, including potential reuse. This add a knowledge-sharing component to the code reviews^[2]
some pre-existing things could have been done better at the same time (boy scout rule)
code follows conventions, doesn't have dead code, doesn't pollute, which arguably should really be done by the tooling

references

[2]: Palantir / Code review best practices

Purpose

Does this code accomplish the author’s purpose? Every change should have a specific reason (new feature, refactor, bugfix, etc). Does the submitted code actually accomplish this purpose?
Ask questions. Functions and classes should exist for a reason. When the reason is not clear to the reviewer, this may be an indication that the code needs to be rewritten or supported with comments or tests.

Implementation

Think about how you would have solved the problem. If it’s different, why is that? Does your code handle more (edge) cases? Is it shorter/easier/cleaner/faster/safer yet functionally equivalent? Is there some underlying pattern you spotted that isn’t captured by the current code?
Do you see potential for useful abstractions? Partially duplicated code often indicates that a more abstract or general piece of functionality can be extracted and then reused in different contexts.
Think like an adversary, but be nice about it. Try to “catch” authors taking shortcuts or missing cases by coming up with problematic configurations/input data that breaks their code.
Think about libraries or existing product code. When someone re-implements existing functionality, more often than not it’s simply because they don’t know it already exists. Sometimes, code or functionality is duplicated on purpose, e.g., in order to avoid dependencies. In such cases, a code comment can clarify the intent. Is the introduced functionality already provided by an existing library?
Does the change follow standard patterns? Established code bases often exhibit patterns around naming conventions, program logic decomposition, data type definitions, etc. It is usually desirable that changes are implemented in accordance with existing patterns.
Does the change add compile-time or run-time dependencies (especially between sub-projects)? We want to keep our products loosely coupled, with as few dependencies as possible. Changes to dependencies and the build system should be scrutinized heavily.

Legibility and style

Think about your reading experience. Did you grasp the concepts in a reasonable amount of time? Was the flow sane and were variable and methods names easy to follow? Were you able to keep track through multiple files or functions? Were you put off by inconsistent naming?
Does the code adhere to coding guidelines and code style? Is the code consistent with the project in terms of style, API conventions, etc.? As mentioned above, we prefer to settle style debates with automated tooling.
Does this code have TODOs? TODOs just pile up in code, and become stale over time. Have the author submit a ticket on GitHub Issues or JIRA and attach the issue number to the TODO. The proposed code change should not contain commented-out code.

Maintainability

Read the tests. If there are no tests and there should be, ask the author to write some. Truly untestable features are rare, while untested implementations of features are unfortunately common. Check the tests themselves: are they covering interesting cases? Are they readable? Does the CR lower overall test coverage? Think of ways this code could break. Style standards for tests are often different than core code, but still important.
Does this CR introduce the risk of breaking test code, staging stacks, or integrations tests? These are often not checked as part of the pre-commit/merge checks, but having them go down is painful for everyone. Specific things to look for are: removal of test utilities or modes, changes in configuration, and changes in artifact layout/structure.
Does this change break backward compatibility? If so, is it OK to merge the change at this point or should it be pushed into a later release? Breaks can include database or schema changes, public API changes, user workflow changes, etc.
Does this code need integration tests? Sometimes, code can’t be adequately tested with unit tests alone, especially if the code interacts with outside systems or configuration.
Leave feedback on code-level documentation, comments, and commit messages. Redundant comments clutter the code, and terse commit messages mystify future contributors. This isn’t always applicable, but quality comments and commit messages will pay for themselves down the line. (Think of a time you saw an excellent, or truly terrible, commit message or comment.)
Was the external documentation updated? If your project maintains a README, CHANGELOG, or other documentation, was it updated to reflect the changes? Outdated documentation can be more confusing than none, and it will be more costly to fix it in the future than to update it now.

[3]: Code smells, see also the Code Smells in Bob Martin's Clean Code.

Gergely Orosz / How to Make Good Code Reviews Better is a collection of recommendations to raise the bar of code-reviews {TODO: extract if relevant}

Smartbear / Best practices for peer code review has some good metrics on how many LOC to review at once (400 max), how long to review code (max 1h), and how fast to review code (500LOC/h max)

Charles-Axel Dein / Maslow's pyramid of code review

idea

links

references