From 2f67cde7cda55f449b4174fc97eab3151f380b19 Mon Sep 17 00:00:00 2001 From: alapha888 Date: Fri, 9 Oct 2026 08:43:22 +0000 Subject: [PATCH] Add code review cheat sheet --- README.md | 1 + tools/code_review.md | 88 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 tools/code_review.md diff --git a/README.md b/README.md index 9041230..a228cca 100644 --- a/README.md +++ b/README.md @@ -119,6 +119,7 @@ Feel free to take a look. You might learn new things. They have been designed to #### Development - [cURL](tools/curl.sh) +- [Code Review](tools/code_review.md) - [Drush](tools/drush.sh) - [Elasticsearch](tools/elasticsearch.js) - [Emmet](tools/emmet.md) diff --git a/tools/code_review.md b/tools/code_review.md new file mode 100644 index 0000000..3575636 --- /dev/null +++ b/tools/code_review.md @@ -0,0 +1,88 @@ +# Code Review + +> Will this code become someone else's problem within three months? + +## Core Rules + +- Walk the five axes in priority order: **Correctness → Security → Readability → Performance → Tests**. +- Each axis is graded **pass / fail / N-A**. +- **Concrete-fix rule**: Every failure must include an exact line reference and a concrete fix. +- **Max 10 comments**: >10 issues means reject and ask for resubmission, not a 50-item grade sheet. +- **No style policing**: Formatting, quotes, and spacing belong to linters. + +## Scope + +| Diff size | Action | +| --- | --- | +| ≤ ~400 lines | Review normally | +| > ~400 lines | Request a split first; review quality collapses on large diffs | + +## The Five Axes + +### 1. Correctness (Blocks merge) +- Edge cases handled: null, empty, zero, negative, boundary/oversized input. +- Concurrency and timing assumptions hold; no race conditions. +- Errors and exceptions handled explicitly, never swallowed. + +### 2. Security (Blocks merge) +- No user input concatenated into SQL, shell commands, or HTML. +- No hard-coded secrets, tokens, or credentials. +- Logs and error messages do not leak sensitive data or PII. + +### 3. Readability +- Descriptive names; functions have a single responsibility. +- Magic numbers are extracted into named constants. +- Flag only incomprehensible logic, not personal style preferences. + +### 4. Performance +- No repeated queries, allocations, or recomputations inside loops. +- No N+1 query patterns across boundaries. +- No data-free speculation ("this might be slow" is invalid feedback). + +### 5. Test Coverage +- New logic and modified paths have automated tests. +- Edge cases and failure modes have dedicated test cases. +- All-green suites that miss critical paths get sent back. + +## Writing Comments + +### Fixed Format + +```text +[severity][axis] file:line problem → suggested fix +``` + +**Examples:** +```text +[Must fix][Correctness] order.py:87 empty order list raises IndexError → guard before taking [0] +[Should fix][Readability] order.py:92 magic number 86400 → name it SECONDS_PER_DAY +``` + +### Triage + +| Label | Axes | Blocks merge? | +| --- | --- | --- | +| Must fix | Correctness, Security | Yes | +| Should fix | Readability, Performance, Tests | No (say so explicitly) | + +## Anti-patterns + +| Anti-pattern | Failure mode | +| --- | --- | +| Drive-by LGTM | Approving before reading the whole diff | +| Style policing | Nits on quotes while logic defects slip through | +| Vague feedback | "This looks off" without line number or fix | +| Speculative perf | Blocking code on unmeasured performance worries | +| 50-item grading | Teaching in review instead of sending back to rewrite | + +## Quick Checklist + +```markdown +- [ ] Scope: diff ≤ ~400 lines (split if larger) +- [ ] Correctness: edge cases handled, exceptions caught, no races +- [ ] Security: input sanitized, no secrets, no sensitive logs +- [ ] Readability: clear names, single responsibility, no magic numbers +- [ ] Performance: no loop queries/N+1, no unverified perf speculation +- [ ] Tests: new paths covered, edge cases tested +- [ ] Fixes: every comment has file:line and a concrete fix (max 10) +```