🔎 Objective
- Establish a shared understanding of what code review is for, so that reviews advance the codebase instead of becoming bottlenecks or rituals.
- In data science especially, ensure code review safeguards the validity of the analysis itself — accurate inference, reusable pipelines, and the transfer of tacit knowledge across the team.
🎯 Goal
📘 Guideline
The 18 principles below are organized into three parts: what code review is for (essence), what to check (checkpoints), and how to behave as a reviewer (discipline). Skipping any one of the three weakens the other two — checkpoints without discipline become nitpicking, and discipline without checkpoints becomes hand-waving.
I. The Essence of Code Review
Discernment, not gatekeeping. The purpose of code review is to verify that new contributions advance the shared codebase without introducing defects, drift, or unnecessary complexity, while preserving its conventions and intent.
Validity of the analysis. In data science, code review additionally protects the validity of the analysis itself: accurate inference, consistent design, reusability of analytical pipelines, maintainability over months of iteration, and the transfer of tacit knowledge across the team.
Code must earn its existence. Discernment means asking whether each line earns its place. Anything that does not serve the author’s stated purpose, the reader’s understanding, or the system’s correctness should be questioned — not accepted out of politeness.
The last cheap moment. A review is the last cheap moment to change direction. Once merged, every line becomes precedent that future code will imitate.
II. Checkpoints
Run through these in order. The earlier ones (Purpose, Correctness, Completeness) gate the later ones — there is no point arguing about abstractions if the intent itself is unclear.
Purpose. Does this code accomplish what the author intended? If the intent is unclear, that itself is a finding — ask for the intent to be made explicit before judging the implementation.
Correctness. Are there obvious logic errors? Read the code as the machine will execute it, not as the author meant it to read.
Completeness. Against the stated requirements, are all cases implemented? Watch for silently dropped branches, unhandled inputs, and TODO debt left behind.
Conformance. Does the code follow the existing style and conventions of the surrounding codebase? A new style introduced in one file becomes drift in the next.
Improvability. Could this be shorter, faster, or clearer without harming intent? Resist gold-plating; only suggest changes that pay for themselves.
Approach. Is this the best way to achieve the desired result, or merely the first that worked? Sometimes a smaller change to a different file removes the need for the diff entirely.
Edge cases. Does the code handle empty inputs, nulls, boundary values, concurrent access, partial failures, and the conditions the author may not have considered?
Abstractions. Do you see useful abstractions emerging from repetition — or premature abstractions hiding simple code? Three similar lines is better than the wrong abstraction.
Tests. Are the unit tests appropriate? Tests should exercise the contract, not the implementation, and should fail loudly when the contract changes.
Documentation. Are comments and docs present where intent is non-obvious — and absent where the code already speaks for itself? A comment that restates the code is noise; a comment that captures a hidden constraint is gold.
III. The Discipline of the Reviewer
Name the good as well as the bad. Calling out a strong choice teaches the team what “good” looks like and reinforces it; silence after merge teaches nothing.
Critique the code, not the author. The diff is the subject; the person is a collaborator.
Prefer questions over verdicts when intent is uncertain. “Why this approach?” surfaces more bugs than “this is wrong.”
A review without a recommended action is incomplete. Approve, request changes, or escalate — never leave the author to interpret silence.