The 7 Code Review Manners

Not the code review we need but the code review we deserve

1. No Ego

That is — if you want the opportunity to learn from others and grow.

2. All your code are belong to us

The code that is being reviewed belongs to, and is a product of, a team. Instead of asking “Wouldn’t you prefer x y z because of 1 2 3?” you can ask “Wouldn’t we prefer x y z because of 1 2 3?”.

3. Style is subjective. Linter is objective.

Instead of repeated, subjective, comments about style — leave it to the linter to be the source of truth.
See people writing code that you really really don’t like (but works)? Great! After convincing everyone that your style is preferable — add a task to improve the linter and reflect that.

4. Readability > Performance (unless proven otherwise)

There is no need to optimize idiomatic code that did not fail. Be careful where you trade-off readability for performance, or you will be left with a very performant but impossible to maintain code.

5. Comments > Change Requests

Managed git services (e.g. GitHub) distinguish Comments from Change Requests. Know the difference between something that is objectively wrong and broken and something that you, subjectively, don’t like. The former is the only reason for a change request. You merely don’t like something? Add a comment. And approve the PR.

6. Praise good code

“I really like this module, its clean and very well tested”. Yes. This is also a part of a good code review. Imagine only hearing the bad things about your code all the time. Wouldn’t that be just terrible?

7. Do not breach scope

This is something I see a lot and I credit git for it. Reviewers tend to review changed lines with already-bad code along with changes the coder did.
While already-bad code should be dealt with, its not practical to involve them in the pull request that merely touched the same lines with them. Even if they’re very small.

Cleaning up these small things should be done in a separate PR. You can even add routine tasks for them and list them as subtasks when you run into them in PRs. Most of the times these housekeeping PRs involve no logic changes, are easily reviewed, merged and deployed — and most importantly — do not block a feature.