Not every developer approaches code review the same way. For instance, some people consider good naming essential while others argue that they don't have time to discuss naming. My take on that is clear: Naming is difficult but important, shapes thinking and helps understanding of code. It's okay to not get names right the first time but if you don't have time to fix bad naming during review, your team is probably in trouble.

I have documented a team's approach to code review more than once so maybe it's time to share my approach to code review as a blog post.

Why do code review?

Why is time spent on code review time spent well? Code review helps…

• to transfer knowledge (both ways),
• to keep code quality up and to prevent introduction of bugs,
• to reduce bias on where to take shortcuts and where to go extra miles, and
• to practice communication.

How to write code that is about to be reviewed?

My rules of thumb for making changes are:

Commits

• [C1] Make things that explain themselves.
• [C2] Comment what cannot explain itself.
• [C3] Resist adding unused features.
• [C4] Add tests for every bug you fix.
• [C5] Add tests for every feature you add.
• [C6] Keep code coverage up.
• [C7] Keep technical debt down.
• [C8] If your energy allows, do bonus clean up in small amounts.
• [C9] Make refactorings, reformatting, bugfixes, new features be separate commits.
• [C10] Delete dead code, Git history has everyone covered.

Branches and pull requests

• [B1] Feed main from pull requests, not direct pushes, to keep the main branch CI green.
• [B2] Merge pull requests with uncontroversial content, passed review and green CI if you are the author.
• [B3] When CI on main turns red, fixing CI is everyone's top priority.
• [B4] Push to branches that you created, not others.
• [B5] Keep branches short-lived to avoid conflicts and ease tool use.
• [B6] Keep branches small to ease review and help branch lifetime.
• [B7] Keep merge commits out of pull requests, rebase against the target branch instead.

How to do review?

During review I am looking for…

• introduction of new bugs,
• things that were forgotten but should not be missing,
• things that are misleading or easy to misunderstand,
• places that cause more cognitive load than necessary,
• things that do not explain themselves and are not commented; applies on all levels, e.g. variable names, class hierarchy, etc.,
• unclear and/or lacking commit messages,
• commits that do more than one thing or more than they should; good Git history helps understanding the past so it better be clean,
• inconsistency with the existing code base,
• common struggles, e.g. proper use of super() in overridden methods.

I am not, or at least a lot less, reviewing for and caring about:

• code formatting — we mostly have CI and pre-commit checks for that —, and
• enforcing the same style on everyone.

Challenges during code review

Things can go sideways when…

• code review starts to replace proper testing,
• authors expect waving through rather than proper review,
• review becomes personal,
• attitude "we do not have time to discuss this" meets someone who cares,
• code quality remains low across review iterations so that the reviewer could have done the whole thing him- or herself better in a fraction of the time and energy,
• pull requests are too big to begin with,
• unreasonable time constraints exist,
• there are larger gaps in knowledge on the side of the reviewer,
• non-obvious claims are made but are not backed up with sources,
• reviewers request clean-ups that exceed the scope of the change and the energy of the author, or when
• multiple reviewers disagree and insist on their point.

Do you want to discuss your code review situation with me? Please get in touch.

Best, Sebastian