Add code review cheat sheet

This commit is contained in:
alapha888 committed 2026-10-09 08:43:22 +00:00
1 parent 38f406dc19
commit 2f67cde7cd
2 files changed
+89

No files matched your search

+88
View File
@@ -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)
```