407c0a560c
apply.sh: - refactor entire script into main() + 14 named functions - fix BSD sed portability: \s → [[:space:]] - fix set -e traps: replace [[ ]] && cmd with if blocks - write_version_file in both scan and update mode - print_summary only in scan mode - keep 3 justified comments, remove all section headers clean-code.instructions.md: - add Newspaper Readability section - add Single Level of Abstraction (SLAB) section - add Declarative and Imperative Layers section - rewrite Comments section (Allowed / Forbidden / The test) - add guard clause guidance to Functions section - add "always use braces" rule to Formatting - add "avoid optical illusions" rule to Formatting - replace Bash/Python examples with TypeScript
199 lines
10 KiB
Markdown
199 lines
10 KiB
Markdown
# Clean Code
|
||
|
||
Based on Robert C. Martin's *Clean Code*. Apply these principles to all code and configuration, including Helm charts, YAML, and infrastructure-as-code.
|
||
|
||
---
|
||
|
||
## Naming
|
||
|
||
- Use intention-revealing names: `daysSinceLastPayment` not `d`
|
||
- Don't lie with names: `accounts` not `accountList` if it isn't a list
|
||
- Pronounceable, searchable names — avoid encodings like `a2dp`, `strName`
|
||
- One word per concept across the codebase: pick `fetch`, `get`, or `retrieve` — not all three
|
||
- Names should read like prose: `if user.isEligibleForRefund()` not `if user.check()`
|
||
|
||
## Functions
|
||
|
||
- **Do one thing** — if you can extract a meaningful function from it, it's doing two things
|
||
- **One level of abstraction per function** — don't mix high-level orchestration with low-level detail in the same function
|
||
- Keep short — aim for under 20 lines; use extract method aggressively
|
||
- 0–2 arguments preferred; use a parameter object when more are needed
|
||
- **No side effects** — a function named `checkPassword` must not also start a session
|
||
- No flag arguments: `delete(file, true)` means the function already does two things — split it
|
||
- **Guard clauses** — use early return for validation/error cases at the top of a function to avoid nesting the main flow in an `else` block. When both branches share code after the split, use `if/else` instead — it reads more clearly and avoids duplication
|
||
|
||
## Newspaper Readability
|
||
|
||
Code should read like a good newspaper article: the headline (function/class name) tells you everything, the opening paragraph gives the high-level story, and the details follow below in order. A reader should understand the whole without jumping around.
|
||
|
||
- **High-level first** — the top-level function appears at the top of the file; it calls lower-level helpers that follow below it in the order they are called
|
||
- **One thought per "paragraph"** — one function = one clear theme; if you need a heading to separate sections inside a function, extract a function instead
|
||
- **Consistent vocabulary** — use the same terms throughout: pick one word per concept and stick to it
|
||
- **Smooth flow** — control flow reads top-to-bottom; avoid hidden state changes and surprising side effects that break the narrative
|
||
|
||
### Newspaper Test
|
||
Ask: *"Can someone read this function for 30 seconds and describe what it does without reading any of the helpers it calls?"*
|
||
If no → the function name or its level of abstraction is wrong.
|
||
|
||
## Single Level of Abstraction (SLAB)
|
||
|
||
One function = one technical level. High-level orchestration and low-level mechanics must not appear in the same function.
|
||
|
||
**Levels of abstraction (examples):**
|
||
- High: business flow — `processOrder`, `setupProject`, `handleUpdateMode`
|
||
- Mid: coordination — `applyDiscount`, `resolveDocsDir`
|
||
- Low: mechanics — `saveToDB`, `readFile`, arithmetic, string manipulation
|
||
|
||
**Bad — mixed levels:**
|
||
```typescript
|
||
function processOrder(order: Order): void {
|
||
if (order.total < 100) { // high-level policy
|
||
const discount = order.total * 0.1; // low-level calc
|
||
order.total -= discount; // mutation
|
||
db.save('orders', order); // external call
|
||
}
|
||
}
|
||
```
|
||
|
||
**Good — single level:**
|
||
```typescript
|
||
function processOrder(order: Order): void {
|
||
const validated = validateOrder(order);
|
||
const discounted = applyDiscount(validated);
|
||
saveOrder(discounted);
|
||
}
|
||
// Each helper owns its own level of detail
|
||
```
|
||
|
||
**AI prompts to enforce SLAB:**
|
||
- *"Refactor this function to a single level of abstraction — high-level calls only, extract all low-level logic."*
|
||
- *"Review this code: does each function stay at one abstraction level? Flag any that mix levels."*
|
||
- *"Generate [feature] newspaper-style: headline function first, detail functions below in call order."*
|
||
|
||
## Declarative and Imperative Layers
|
||
|
||
Well-structured code has a clear layer boundary between *what* and *how*.
|
||
|
||
- **Top layer — declarative**: reads like a description of intent. No loops, no conditionals on mechanics, no string manipulation. A non-programmer should be able to read it and understand the flow.
|
||
- **Middle layer — still mostly declarative**: coordinates steps and error paths, calls lower helpers. May have simple conditionals about *what* to do, not *how*.
|
||
- **Bottom layer — imperative**: the actual mechanics. `grep`, `awk`, `find`, SQL, arithmetic, string parsing. This is the only layer where implementation details belong.
|
||
|
||
```typescript
|
||
// Top — declarative (what happens)
|
||
async function runCli(args: CliArgs): Promise<void> {
|
||
const config = await loadConfig();
|
||
if (args.updateOnly) {
|
||
await handleUpdateMode(args.prevCommit);
|
||
} else {
|
||
await scanProjects(config.devRoot);
|
||
printSummary();
|
||
}
|
||
writeVersionFile(config.devRoot);
|
||
}
|
||
|
||
// Middle — declarative (what each project needs)
|
||
async function setupProject(project: Project): Promise<void> {
|
||
ensureSymlink(project);
|
||
ensureGitignore(project);
|
||
const docsDir = await resolveDocsDir(project);
|
||
if (docsDir) await ensureDocFiles(docsDir);
|
||
}
|
||
|
||
// Bottom — imperative (how a symlink is actually created)
|
||
function ensureSymlink(project: Project): void {
|
||
const linkPath = path.join(project.path, '.ai');
|
||
if (fs.existsSync(linkPath) && fs.readlinkSync(linkPath) === AI_TARGET) return;
|
||
fs.symlinkSync(AI_TARGET, linkPath);
|
||
}
|
||
```
|
||
|
||
**Test:** Can you cover the bottom-layer functions and still understand the program from reading only the top two layers? If yes, the layering is correct.
|
||
|
||
## Classes
|
||
|
||
- **Single Responsibility Principle** — one reason to change
|
||
- Small and cohesive — methods should use the class's fields, not act as utilities
|
||
- Hide internals: public API simple, private implementation complex
|
||
- No god classes — prefer `InvoiceCalculator` over `BusinessLogic`
|
||
|
||
## Error Handling
|
||
|
||
- Throw exceptions, don't return error codes — let callers handle what they understand
|
||
- Descriptive messages: `throw new UnauthorizedAccess("Admin role required for this action")`
|
||
- Define custom exceptions close to where they're used
|
||
- Clean up resources: use try/finally or RAII patterns; never leak
|
||
|
||
## Comments
|
||
|
||
**The default is no comments.** If you feel the urge to write one, first try to eliminate the need by renaming or extracting a function.
|
||
|
||
A comment is only justified when the code cannot speak for itself — meaning the *why* is non-obvious and cannot be expressed in the language itself:
|
||
|
||
**Allowed:**
|
||
- A bug or quirk in a dependency forces unusual code: `# stripe returns 402 for both expired cards and fraud — we treat both as declined`
|
||
- Non-obvious language or platform behaviour: `# BASH_SOURCE[0] is empty when piped through bash`
|
||
- Timing or ordering constraint that isn't visible from the code: `# capture before exec replaces the process`
|
||
- A trap for the next developer about a hidden coupling: `# sets got_new=1 via dynamic scope — caller must declare 'local got_new'`
|
||
|
||
**Forbidden:**
|
||
- Describing what the function name already says: `# load docs folders` above `load_docs_folders()`
|
||
- Section dividers that replace missing structure: `# ── Config ──────────`
|
||
- Repeating the parameter contract in prose: `# Sets RESOLVED_DOCS_DIR; returns 1 if user declines`
|
||
- Any comment that answers "what" instead of "why"
|
||
- TODO comments left in merged code
|
||
- Commented-out code
|
||
|
||
**The test:** Cover the function name and read only the comment. If someone could have written that comment without reading the code, it adds no value — delete it.
|
||
|
||
## Formatting
|
||
|
||
- Related code stays close — declarations near first use
|
||
- Consistent indentation throughout the codebase — agree once, automate with linting
|
||
- No deep nesting — if you're 4 levels deep, extract a function
|
||
- Team standard beats personal preference every time
|
||
- **Always use braces for `if`, `for`, `while`** — even for single-line bodies:
|
||
```typescript
|
||
// Bad
|
||
if (docsDir) await ensureDocFiles(docsDir);
|
||
|
||
// Good
|
||
if (docsDir) {
|
||
await ensureDocFiles(docsDir);
|
||
}
|
||
```
|
||
Single-line bodies invite accidents when a second line is added later.
|
||
- **Avoid optical illusions** — code that looks like it does one thing but does another is the most dangerous kind of bug. Common sources:
|
||
- Side effects hidden in expressions: `if (user = getUser())` or `list.sort()` that returns void
|
||
- Boolean logic that reads as simpler than it is: `!a || !b` — write `!(a && b)` or extract `isInvalid(a, b)`
|
||
- Chained calls hiding control flow: `items.filter(...).map(...).forEach(save)` — if `save` throws, the chain silently stops
|
||
- Formatting that suggests grouping: unbraced `if` followed by two lines where only the first is conditional
|
||
- Names that imply the wrong type or behaviour: `getUserList()` returning a `Promise`, `isReady` that triggers a network call
|
||
|
||
## General Principles
|
||
|
||
- **DRY** — every piece of knowledge has one representation; duplication is a maintenance liability
|
||
- **KISS** — the simplest solution that works; complexity must earn its place
|
||
- **Boy Scout Rule** — leave the code cleaner than you found it; small improvements compound
|
||
- **SOLID** — especially SRP (above) and Open-Closed: open for extension, closed for modification
|
||
- **Boundaries** — wrap third-party code in adapters; don't let external APIs bleed through the codebase
|
||
|
||
## Testing
|
||
|
||
- Tests must be **FAST, Independent, Repeatable, Self-Validating**
|
||
- One concept per test — a test that asserts many things is a test that hides failures
|
||
- Keep test code as clean as production code — it will be maintained just as long
|
||
- Write code testable from day one; retrofitting testability is expensive
|
||
|
||
---
|
||
|
||
## In Helm Charts and YAML
|
||
|
||
The same principles apply to configuration:
|
||
|
||
- **Naming**: value keys should be intention-revealing — `replicaCount` not `rc`, `service.port` not `p`
|
||
- **Single responsibility**: one chart does one thing; don't bundle unrelated services
|
||
- **No magic values**: named values in `values.yaml`, never hardcoded in templates
|
||
- **DRY**: use template helpers (`_helpers.tpl`) to avoid repeating label blocks and name patterns
|
||
- **Comments**: only when a value's *why* is non-obvious — a workaround for a known bug, a constraint imposed by an external system, or a counter-intuitive default that will surprise the next person
|
||
- **Defaults that make sense**: a default of `false` or `""` that always needs overriding is not a useful default
|