# Clean Code Principles (language-agnostic) Apply these on every change. They override personal taste. They do **not** override project-local rules. ## DRY — Don't Repeat Yourself A second copy of the same logic is a signal to extract. Two callers of the same algorithm should call the same function. Bad: ```javascript let total = 0; for (let i = 0; i < prices.length; i++) total += prices[i]; // later, elsewhere: let t = 0; for (let i = 0; i < prices.length; i++) t += prices[i]; ``` Good: ```javascript const sum = (arr) => arr.reduce((acc, x) => acc + x, 0); ``` Caveat: do not extract until the duplication is real (rule of three). Premature abstraction is also a code smell. Two more forms of duplication that hide from symbol-level grep: - **Scattered domain literals** — the same enum value / predicate repeated across queries or filters *may* be one business decision duplicated. The test: would the sites change together when the rule changes? If yes, consolidate to a named constant or builder at a single reference point; if they merely share a value, leave them apart — coupling incidental duplication is worse than repetition. - **Documented forks** — two deliberate variants of one capability kept as parallel implementations "because the delta is commented". If the delta is genuinely parametric (a few orthogonal options), collapse into one implementation with explicit option parameters; if not, split into two distinctly named capabilities. The commented fork itself is never the end state. ## KISS — Keep It Simple The simplest solution that meets the requirement wins. Cleverness has a maintenance cost. Bad: ```javascript if (data !== null || data !== undefined || data !== '') { ... } ``` Good: ```javascript if (data) { ... } ``` ## SOLID - **S**ingle Responsibility — one reason to change per class / module. - **O**pen/Closed — open for extension, closed for modification. Prefer composition. - **L**iskov Substitution — a subtype must honour the parent's contract. - **I**nterface Segregation — many small interfaces beat one fat one. - **D**ependency Inversion — depend on abstractions, not concrete implementations. Bad (SRP): ```python class User: def save(self): ... def send_email(self, msg): ... ``` Good (SRP): ```python class UserRepository: def save(self, user): ... class EmailService: def send(self, user, msg): ... ``` ## YAGNI — You Aren't Gonna Need It Implement what is required **now**. Do not add knobs, options, hooks, or layers for a hypothetical future caller. Bad: ```javascript function calculator(a, b, op) { switch (op) { case 'add': return a + b; case 'multiply': return a * b; // not asked for case 'divide': return a / b; // not asked for } } ``` Good: ```javascript const add = (a, b) => a + b; ``` ## Meaningful naming Names reveal intent. `d` and `crtUsrAcct()` are bugs; `elapsedTimeInDays` and `createUserAccount()` are documentation. ### Names must be truthful The name must describe what the function actually does. Misleading names break reader expectations and survive renames. Flag and rename: - `get*` / `fetch*` / `find*` that also mutate state, throw, or write logs / audits. - `is*` / `has*` / `can*` that throw instead of returning a boolean. - `find*` that *creates* the entity when missing → `findOrCreate*` or `ensure*`. - Names that describe the implementation, not the intent: `loopOverUsers` → `notifyActiveUsers`, `runQuery` → `listOverdueInvoices`. - Names that say the opposite of what they do under some branch: `enableFoo` that disables when a flag is off, with no hint in the name. ### Names must stand alone The test: if the name appeared in a stacktrace, an autocomplete list, or a grep result, would the reader know what it does without opening the file? If not, add specificity. - Names missing the noun: `createPending()` → `createPendingInstallation()`. The verb is fine; the object is missing. - Generic verbs with no information: `handle`, `process`, `execute`, `doStuff`, `manage`. If the function deletes-then-inserts, name it `replace`, not `update`. - Constants that will collide with siblings later: `BUCKET` → `FONTS_BUCKET`, `TIMEOUT` → `HTTP_READ_TIMEOUT_MS`, `URL` → `AUTH_SERVICE_URL`. - Repository / port methods named after the rule they enforce, not the data they fetch: `findValid*` / `findActive*` / `findEligible*` — the adjective encodes a business rule the caller can't inspect from the name. Prefer `findDesktopLibrariesForUser(userId)` + a domain predicate applied by the caller. - Near-twin siblings a caller can't tell apart from the names alone (`parseRows` vs `parseRowsFromItems`): if a comment must explain which to pick, rename — encode the distinguishing fact (e.g., input shape: `parseRowsFromFlatItems`). Matching the existing sibling's style does not excuse the ambiguity. This rule applies most strongly to **names crossing module boundaries**: public methods, exported constants, port methods. Private helpers in a tight local scope get more slack. ### One identifier, one meaning per file A name that means two things in one file makes every reader re-derive which one they are looking at. The common shape: a new parameter holding a key takes the name a sibling signature already gives to the whole entity. Bad: ```ts findFullAccount(account: string) // a primary key update(idAccount: string, account: AccountRow) // the row — same word, four lines away ``` Good: `findFullAccount(idAccount: string)` — the key takes the name the file already uses for keys. The test: grep the identifier across the file. If two occurrences carry different types or different roles, rename the newer one to the distinguishing fact and follow whatever name the file already uses for that role. ## Functions do one thing If the name contains "and", or you must scroll to read it, split it. Bad: ```javascript function handleData() { fetchData(); processData(); displayData(); } ``` Good: each step is its own function with a clear name and signature. ### Plain-English summary test For every function you add or meaningfully modify, ask: > Can I summarize what this function does, line by line, as a short English sentence per line — and does the resulting summary read as prose? If the answer is "no, I'd have to group several lines together and invent a name for the group", **that grouping should already exist in the code as a named helper or named intermediate value**. The fact that you had to invent the name is the signal to extract. ### Also flag and split - Nested conditionals 3+ levels deep — flatten with early returns or named predicates. - Mixed levels of abstraction — a high-level orchestrator suddenly doing string parsing, date arithmetic, or SQL assembly inline. - A reader has to track 5+ anonymous temporary values to understand the return. - A long `.map().filter().reduce()` chain where each stage does non-obvious work and no stage has a name. - Boolean expressions long enough that intent is buried — `if (a && b && !c && (d || e.f > 0) && ...)` without a named predicate. - A function that orchestrates 4+ distinct logical phases inline (*fetch → resolve → branch → persist → publish*), each more than a line or two. Even without deep nesting, that's enough work to warrant named sub-methods. - A chain of `if (x === SomeEnum.A) return …` over the values of one enum — that's a hand-rolled switch. Use a real `switch (x)` so the cases line up and exhaustiveness is visible; it also drops the trailing fall-through `return` that an if-chain needs. - A dense data literal — an object/struct/`{...}` carrying several inline ternaries or conditional spreads (`...(cond ? { x } : {})`) — assembled inline in a method that also does other work. Extract a named builder (`buildX(...)`) so the method reads as "compute X → assign X", and each conditional field gets a place to be understood. ### 50-line cap A single function/method body must stay within **50 lines**, counting only effective code (exclude blank lines, comments, and pure data declarations such as large enums, lookup tables, or constant maps). Crossing the cap is an extraction signal, not a style nit. Before declaring a function complete, count effective body lines; if over 50, split, or surface the violation and confirm with the user before continuing. ### What's fine - Functions that read as a straight sequence of clear, already-named steps (each line is an English sentence). - Short functions (< ~10 lines) regardless of shape. - Idiomatic framework patterns used clearly (a 30-line controller method obviously doing its job). - Guard clauses at the top — they *help* readability, they don't count against you. ## Mutation and state boundaries These defects survive a green suite because the test reads the same object the code mutated. State the fact you actually verified, at the moment you verified it. ### Decide on the direct fact, not a proxy A status field, a flag, or a "was processed" marker is evidence *about* a decision, not the decision itself. When the decision turns on identity or a destination — did this row move, did ownership change, is this the same target — compare the source and destination identifiers directly. If the direct fact is unavailable, do not perform the change; surface it. The test: construct the opposite case and ask whether your condition still reads true. `status === 'DONE'` holds both for "already moved here" and "moved somewhere else"; `row.parentId === target.parentId` does not. ### Capture before-state before the mutating boundary When a value describes the state *before* processing, read it before anything that can mutate the object — a strategy call, a repository write, a reload, any `await` that hands the object to other code. Read every before-value feeding the same diagnostic in **one snapshot, at one moment**. Never re-read a before-field off the original object after that boundary, and never reconstruct it from the post-state. Pass before-state and result-state as two explicit values. ### Update only the fields this work owns Creating a new row and updating an existing one are not the same write authority. When reusing an existing row, update only the fields the current operation owns, and express them as an allowlist. Provenance, ownership, and externally-managed values are not overwritten without an explicit requirement saying so. Keep the create-vs-reuse answer available at the later write step — that is what tells the two authorities apart. The test: for every field this change writes, grep the other sites that write the same field in the same flow — a later pass of the same loop, a final persist, a sibling handler — and put them in order. If a later write clears what an earlier one recorded, the clear needs a condition, not a default. ### Zero affected rows is a question, not an answer A conditional write that changed no rows has not told you whether it succeeded. Re-read the current state and separate the three cases: already in the desired state, changed to a different conflicting state, target deleted. Each gets its own success/failure handling and its own message — collapsing conflict and deletion into one error hides the case the caller must act on. ### Priority between competing inputs is policy, not an if-chain Which of several inputs wins is a business rule. It does not belong in a service's early return or inline condition, where the next caller re-derives it differently. The service collects inputs; a named domain function decides. The function's name alone should tell you the policy. ### An error message states only what you observed Do not assert a cause the code never checked — "target not found" after a write that only reported zero rows is a guess. Failures with different remedies (deleted, state mismatch, concurrent change) get different messages, each covered by a test on that path. ## Wrapping a third-party call ### The wrapper must add something the library does not already do Before writing recovery, retry, or fallback code around a library call, read that library's own handling of the same case in its installed source. A `catch` that repeats what the library already did — same read, same transaction, same conditions — recovers nothing; it only replaces a typed error with a weaker one. The test: name the condition under which your branch runs and the library's does not. If you cannot name it from the library's source, the branch is dead. Delete it, or make it do something the library provably does not — a locking read where the library read without a lock, a different isolation level, a retry the library never attempts. ### A comment may not name a condition the code does not run under Recovery documented as "under REPEATABLE READ this re-read succeeds" is false when every caller opens READ COMMITTED. Cite the call site that establishes the condition, or drop the claim — an unverifiable comment outlives the code it excuses. ### Rethrowing keeps the original error Replacing a typed library error with a bare one throws away what the caller needs in order to act: which constraint fired, the driver's message, the original stack. Keep the original as the cause (`new Error(msg, { cause: err })`, `raise X from err`) and keep the type when a caller branches on it. ## Testing discipline These principles apply to every test file regardless of language or framework. The mechanical detection patterns (which mock library, which spy API) are in each `languages/*.md` Tests section. ### No self-mocking A unit test for class `Foo` must **not** stub, spy on, or replace methods on the instance of `Foo` being tested. Mocking injected collaborators (other services, repositories, adapters, ports, the clock) is fine and expected. Why it matters: when you mock a method on the subject under test (SUT), the test no longer verifies that method's behavior — it verifies that *some* version of the class (the one you wired up) calls the mocked method. If someone deletes the real implementation, the test still passes. The test has become a proof of its own shape. Bad (pseudo, language-agnostic): ``` sut = new Foo(deps) mock(sut.calculateTotal).returns(100) // mocking the SUT's own method result = sut.checkout() assert result.total == 100 // proves wiring, not logic ``` Good: ``` sut = new Foo(deps) // real Foo, real calculateTotal mock(deps.payment.charge).returns(ok) // mock the collaborator at the boundary result = sut.checkout() assert result.total == expectedTotal // proves the logic ``` ### Behavioral, not implementation tests Assertions should be about **outcomes** — return values, thrown errors, persisted state, published events, calls made to genuine external boundaries. They should **not** be about *how* the work was done internally — which private helper was called, in what order. Smells: - `expect(spy).toHaveBeenCalledWith(...)` as the *only* or *primary* assertion, when the spy is on an internal helper or a pure collaborator with no side effects. - Tests that would break if you renamed a private method without changing behavior. - Assertions on private methods reached via reflection, cast-to-any, or bracket access — the issue is *reaching into privates*, not the language escape hatch itself. - A test file where most assertions are on spies rather than on results. Legitimately behavioral (these are fine): - `expect(mailerSpy).toHaveBeenCalledWith(...)` — sending mail *is* the behavior. - `expect(paymentGateway.charge).toHaveBeenCalledWith(...)` — charging a card *is* the behavior. - `expect(orderRepo.save).toHaveBeenCalledWith(...)` in an application-service test — producing the right save call *is* the point of the service. The judgment call: is the mocked thing an **outcome boundary** (port, external service, side-effecting adapter) or an **internal helper**? Asserting on boundaries is behavioral; asserting on helpers is implementation-coupled. ### Assert the last write, not the first When one run writes the same record more than once — an update followed by a final persist, two passes of the same loop over sibling rows — asserting the earlier call proves nothing about what the record ends up holding. The later write can drop the very field the test asserted and the test stays green, so the suite certifies a broken end state. The test: list every write in this flow that touches the field you assert, and confirm the one you assert is the last. If it is not, assert the last one too. ### An effect under its own mock is not evidence When a mock replaces the code path that would produce an effect, a test above that mock has not verified the effect — it verified that the mock was reachable. Claim coverage for an effect only where the real path runs; the presence of a mock in the harness is not proof that the branch behind it works. ### Every branch you add gets a test that dies with it A new `catch`, guard, early return, or `else` is a behavior claim. Coverage for it means one test that FAILS when the branch body is deleted — not a happy-path test that merely reaches the same method. The test: delete the branch body and run the suite. Green means the branch is untested, and an untested recovery path is indistinguishable from one that cannot fire at all. This binds hardest on error-recovery code, where the happy path is what the library already does and the branch is the only behavior your code adds. ### Shared fixtures keep the ordinary default A shared fixture's defaults describe the normal path. Do not flip a default to exercise one exception — every existing caller silently changes meaning. Pass the exceptional condition explicitly in the test that needs it (`makeFamily({ familyEnabled: false })`), and search all call sites before changing any default. The test: does the no-argument fixture call still describe the ordinary case? ### Test data must actually separate the paths Two scenarios whose setup values and assertions are identical are one test with two names. Give source and destination, existing and incoming, success and conflict deliberately different values. The check: state each test's distinguishing input in one sentence, then delete that difference — if the test still passes, it never tested the path. A wrong implementation must not pass because the defaults happened to agree. ### A test title names the unit and the one condition it isolates A test title is a name, so the truthful and stand-alone rules above apply to it. From a failure line alone the reader must know which unit ran and which single condition was under test. - Name the unit when the file covers more than one. `'returns the primary-key lookup result'` fits every by-key read in the class; `'findFullAccount returns …'`, or a `describe('findFullAccount', …)` wrapper, fits one. - Credit only the condition the case actually isolates. A parameterised case that trips two guards at once must not name one of them in the title — split it into one case per guard. The test: delete from the setup the input the title credits. If the case still passes, the title describes a guard the test never reached — retitle it, or fix the setup so it does. ### Test tooling you add is used in the same change A new mock, state setter, or repository branch that no test calls is unfinished work, not coverage. Grep every new test identifier for a use site before claiming done, and check that a repository mock does not only ever return success. ### Resource bounds need their own regression test A change to memory use, concurrency, batching, or stream handling is not covered by a functional test that passes on a small input. Add a test that pins the bound the change claims to hold — peak size, concurrent count, chunk count. ## Trace what this change can do wrong The rules above name defect shapes. A defect with no name on this list is still a defect, and the ones that reach production usually have no name — they are ordinary code that produces a wrong result for one input nobody walked. For every source file this change touches, follow the paths the change creates or alters to their end, and state where a wrong result comes out: - **error** — what the caller sees when each new call fails, and whether that is distinguishable from the other failures it must be told apart from. - **partial** — the change succeeded halfway; what is left written, and what the next run sees. - **concurrent** — something else is still writing, or the deadline fired and the work did not stop. - **selection** — when several candidates fail, which one's evidence survives. **A finding names the input or state that produces the wrong result.** *"If the archive yields no entries, line 42 reports success and stores an empty result"* is a finding. Code that works as written is `clean`, however you would have written it differently: alternative structures, guards for states no caller can reach, extra tests for covered paths, and "consider extracting / renaming / memoizing" are improvements, not findings. A real failure that is small and cheap to fix is still a finding. ## No magic numbers Replace hardcoded values with named constants. Bad: `if (age > 18)` Good: `const LEGAL_AGE = 18; if (age > LEGAL_AGE)` ## Limit nesting depth Flatten with early returns / guard clauses. If you reach four nested blocks, refactor. Bad: ```javascript function process(user) { if (user) { if (user.isActive) { if (user.hasPermission) { // ... } } } } ``` Good: ```javascript function process(user) { if (!user) return; if (!user.isActive) return; if (!user.hasPermission) return; // ... } ``` ## Comments explain *why*, not *what* The code already says **what**. Comments record the hidden constraint, the bug being worked around, the surprising tradeoff. Never narrate the next line. Bad: ```javascript // increment i i++; ``` Good: ```javascript // User may be soft-deleted; invalidate cache so stale reads vanish. cache.invalidate(user.id); ``` **A comment is not a change log.** Never record change history in code — ticket IDs (`FU-003`, `DEV-9185`), verification logs (`verified parity-neutral on the real DB`), `retained from …`, or the reason something *changed*. That belongs to git blame, the commit message, and the PR; the code reader six months later does not have those tickets open. A comment earns its place only by stating a constraint or invariant that is **still true and load-bearing for someone reading the code cold, with zero knowledge of the diff that introduced it**. If it stops making sense once that diff is forgotten, delete it. Bad (change-log noise — delete): ```javascript // FU-003 read-path deltas — verified parity-neutral against the publish grid // on the real DB (DEV-9185 Stage 1). Retained as read-side display alignment. applyYearlyMultiplier(rows); ``` Good (constraint a cold reader needs): ```javascript // Publish callers omit both fields, so the 0 multiplier is intentional here. applyYearlyMultiplier(rows); ``` Default to writing **no comment**. Only add one when removing it would confuse a future reader. ## Boy Scout Rule Leave the file cleaner than you found it. Fix one small inconsistency on the way through, do not embark on a separate refactor.