Design Review Simulator
A queue of things a reviewer actually sees: unclear ownership, hidden state, an unsafe migration, an abstraction nobody needed, a failure nobody handled. Each one shows what it looks like and what it suggests. What to do about it — including the case for doing nothing — is behind the button.
A smell is a question, not a verdict. Every one of these has a case where the code is genuinely right as it stands, and a reviewer who cannot state that case is not reviewing — they are pattern-matching. So when this is fine is revealed with equal weight to the fix, and it is worth reading even when you were sure. The most expensive review comments in a codebase are the ones that were technically correct about a smell and wrong about this instance of it.
what you see in the diff A raw Date, string or number compared against another raw value to decide behaviour: if (event.at > since), if (row.tenant === user.tenant), if (amount > limit). No type distinguishes a UTC instant from a local date, an internal id from an external one, or cents from dollars.
what it might indicate A hidden requirement has been answered implicitly. Somebody decided that these two values are comparable, and that decision is recorded nowhere — so the next person who supplies one of them from a different source will supply the wrong kind and nothing will object.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff Reads that repair: const method = sub.paymentMethod ?? await findAnyMethod(sub.customerId). Null checks on fields the schema says are required. A comment reading "shouldn't happen but does". A nightly job whose name contains fix, cleanup or sync. Filters like WHERE payment_method_id IS NOT NULL in reports that are supposed to cover all active subscriptions.
what it might indicate Somebody has already met data that violates the invariant and has coped with it locally rather than reporting it. Each such site both hides the leak and makes it harder to close, because the invalid data now has code depending on being tolerated.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff One class or module that most of the codebase imports, whose method list reads like a table of contents for the whole feature area, and whose constructor takes six or more collaborators.
what it might indicate Responsibilities accreted here because it was the place with the data already loaded. Expect several unrelated external triggers, a blast radius far larger than any individual change, and a test file that has to construct the world.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A class whose methods have the same names as the layer beneath, take the same arguments, return the same values, and contain a single line that forwards the call. Often a Manager between a Service and a Repository, or a DTO that is a field-for-field copy of the entity.
what it might indicate The layer was added because the architecture diagram had a box, not because a decision needed a home. Cost is paid on every change: each new field is edited in every layer, and each layer brings a test file that asserts the forwarding works.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A type whose name is built from Manager, Helper, Util, Handler, Processor, Factory or Impl, often two or three of them together. Its methods are a grab bag with no common subject, and its callers pass it whatever it needs each time.
what it might indicate No model exists. The unit was created because code needed somewhere to go, so it is defined by its mechanism rather than by what a caller may stop thinking about. Expect it to grow monotonically and to be imported everywhere (The Utility Dumping Ground).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A widely-imported function or class whose options object has grown boolean by boolean; each flag is set by one or two callers; at least one flag controls two unrelated behaviours; and the tests enumerate flag combinations rather than describing outcomes.
what it might indicate The abstraction was extracted before the callers' requirements were known and has been held together with switches ever since. Expect invisible coupling between callers, a combinatorial space of untested states, and a change cost dominated by regression rather than by the edit (Boolean Flag Explosion).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A private collection or mutable object handed straight back: getShifts(): Shift[] { return this.shifts }. Callers iterate it, and one of them eventually sorts, splices or pushes.
what it might indicate The module owns a rule it cannot enforce, because there is a route into its state that does not pass through its operations. Expect the invariant to hold in tests and fail in production, at the one call site nobody thought of.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A page of per-module coupling figures with red and green cells, or a build step that fails when a derived instability or complexity figure crosses a line. Often accompanied by a quarterly goal to reduce it.
what it might indicate That a real concern about structural decay has been converted into the most measurable proxy available, and that the proxy will now be optimised. Expect facades, re-export modules and splits that move edges around, and expect the genuinely expensive coupling to stay invisible because it produces no edges (Facade).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff PaymentService and PaymentServiceImpl in the same package, one implementing the other, every method identical, and the interface changing in the same commit as the implementation every single time.
what it might indicate The interface was added because "classes have interfaces", not because a boundary was needed. It is not inverting anything — both files are on the same side — and it forces every reader to make an extra hop with no information gained.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A constructor taking eight or more dependencies — typically a repository, a gateway, a mailer, a cache, a metrics recorder, a feature-flag client, a clock and an event publisher — in a class named SomethingService.
what it might indicate The class has accumulated a use case at a time and now owns several unrelated reasons to change. The dependency count is a proxy for the responsibility count, and it is a good one because it is mechanical and cannot be argued with (Divergent Change).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A class whose every method is one line: return this.next.doThing(x). Often named ...Manager, ...Handler or ...Coordinator, often introduced during a split, often with an interface of its own.
what it might indicate A decomposition that went past the point of usefulness. The class adds a name and a file and no behaviour, so every reader pays navigation cost for nothing, and the "responsibility" it holds cannot be described without using the word "coordinates" (Over-Decomposition).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A directory of interfaces each declaring a single method, typically named SomethingDoer or ISomethingHandler, each with exactly one implementation, and consumers whose constructors list four or five of them.
what it might indicate ISP applied as a size rule rather than as a coupling argument. The coupling was not reduced — the same consumer still depends on the same operations — it was just spread across more type names, and the codebase lost the ability to describe what a consumer is for (How SOLID Gets Misused).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff OrderService.findById(id) { return this.repo.findById(id) }, plus an OrderDto with the same fields as Order and a mapper between them. Repeated across every entity in the system, and often with an interface in front of each.
what it might indicate A layer added because the architecture diagram has one, rather than because something happens at that boundary. It multiplies the edit count of every additive change by the number of layers while adding no decision, no protection and no translation (Package by Layer).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff An override whose body is throw new UnsupportedOperationException(), return null, or an empty block — often with a comment saying it does not apply to this subtype.
what it might indicate The base contract is wider than the subtypes genuinely share. The class was extended to reuse the parts that fit, and the parts that did not were disabled rather than the relationship reconsidered.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A mixin whose methods reference self.id, self.tenant or self.save() without any declaration that a host must provide them — often with a docstring saying "must be used with Model".
what it might indicate An implicit inheritance relationship. The mixin is a base class that avoided the parent slot, and the contract it depends on is enforced by nothing but the reviewer's memory.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A class or function whose entire body is return new Thing(a, b, c) — the arguments passed straight through, no branch, no validation, no lookup, and exactly one type ever returned.
what it might indicate A convention applied without its condition. Somebody was told not to call new, or a code generator produces one of these per entity, and nobody has asked what decision is being encapsulated.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff order.status = "paid" in a repository, an admin action, a migration or a test helper — anywhere other than the transition function. Often justified as fixing bad data.
what it might indicate The state machine is advisory. Every rule, guard and forbidden transition it declares is enforced only for the callers who chose to go through it, which is a documentation-level guarantee wearing a type-level costume (Invariant Leaks).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A class whose every method forwards to the same collaborator with the arguments unchanged, sometimes renaming a field on the way. Its tests assert that the forwarding happened.
what it might indicate A boundary was drawn where nothing differs. It costs a file, a name, a hop, a set of tests and a place for the next engineer to add "just one small thing", and it makes every change touch one more file than it needs to (Change Amplification).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A function whose whole body is a field-for-field copy into a differently-named type — toStockHold(auth) mapping five fields onto five fields with no transformation — plus a display layer that renames it again.
what it might indicate One concept has picked up a second name because two modules were written by different people at different times. Every requirement about the concept now has to be applied twice, and the two copies drift.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A class called AccountService with transfer, closeAccount, exportStatementPdf, recalculateInterest and syncWithFinanceSystem on it, taking six constructor dependencies, and imported by nine modules that each use one method.
what it might indicate The class is named after a data type rather than an operation, so it attracts anything that mentions accounts. Its dependency list is the union of five unrelated needs, which means every test of any one operation drags in all of them, and every change to any of them risks all five.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff An admin endpoint POST /orders/:id/state that writes any state to any order, guarded only by a role check. Usually added after an incident, usually with a comment saying it is for emergencies, usually appearing in the audit log dozens of times a week within a year.
what it might indicate The legal paths do not cover a real operational need, and rather than modelling that need the team added a way around every rule at once. Every forbidden transition is now reachable, so the forbidden list documents intentions rather than behaviour, and the effects that transitions normally trigger are skipped — the state moves and the refund, the reservation and the notification do not.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff Three or more boolean columns or fields whose names are past participles — isPaid, isShipped, isCancelled, isArchived — set by different code paths, and queries that test several of them together with informally-agreed combinations.
what it might indicate One mutually-exclusive lifecycle has been spread across several independent fields, so the representable state space is far larger than the designed one and no single code path can be identified as wrong when an illegal combination appears.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A value from the type's own domain reserved to mean "not there": 0, -1, "", 1970-01-01, 9999-12-31, "N/A", or a magic id like user_id = 0. Usually accompanied by a comment on the column and nothing enforcing it.
what it might indicate Absence was encoded rather than expressed, so every operation defined on the type will accept the sentinel and compute with it. The failure is silent, arrives at aggregation time, and is discovered by a human noticing an implausible number rather than by any check in the system.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A try wrapping a block that contains both a call to something external and a chunk of your own logic, with a catch that logs and continues to the next item.
what it might indicate Nobody has decided which failures are expected. The block catches the provider timeout it was written for, and also every NullPointerException, every KeyError and every impossible-state assertion in the logic that happens to sit inside the same braces — so defects are quietly reclassified as data problems.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff An empty catch; a catch that logs at warn and continues; an unawaited promise; an ignored return value; or — the version with no catch block at all — an operation that returns a boolean when four things happened.
what it might indicate The interface cannot express the state that actually occurred. Somebody needed "done, except for the email" and the only values available were true and false, so the truth went into a log line or nowhere. The catch is the symptom; the impoverished return type is the design failure.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A method returns an internal array, map or object directly; or a constructor stores an argument without copying it; or a closure captures a buffer that outlives the function that made it.
what it might indicate Ownership was never decided. The object still behaves as though it owns the value, and so does the caller, so both will mutate it and neither will expect the other to. The bug appears far from both, as a value that changed for no visible reason — which is the receipt bug from Immutability, one level up.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff One function well past what the surrounding code averages — a few hundred lines, often with sections separated by blank lines or comment banners.
what it might indicate Possibly several responsibilities in one place; possibly deep nesting and a large set of simultaneously live variables; possibly a domain concept with no name. Possibly none of these.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A line-by-line narration — // increment the counter, // loop over users — or a doc block that lists the parameters again with no added information.
what it might indicate Either that the comment is pure noise, or that the code beneath it is unclear enough that the author felt they had to translate it — in which case the naming or the structure is the real finding (Naming).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff max-lines-per-function: 25, max-params: 3, max-depth: 3 and a cyclomatic-complexity ceiling, all at error severity — plus a scattering of inline suppressions on the functions where the team decided the rule was wrong.
what it might indicate A real design argument that was settled by turning one position into a threshold rather than by convincing anyone. The tell is the suppressions: they mark exactly the cases where the team disagrees with its own rule, and nobody has taken that as evidence about the rule. The usual response to a violation is to split a coherent function into two incoherent halves, or to bundle three parameters into an Options bag with no meaning, both of which satisfy the number and worsen the code (Introduce Parameter Object).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A method that reads six fields from order and three from order.shipTo, and two from itself. if (order.total > 5000 && order.items.every(i => !i.hazardous) && order.shipTo.country === 'GB' && ...).
what it might indicate The knowledge of what those fields mean together lives outside the object that owns them. Every field it reaches for is now part of Order's public shape and can never be renamed or restructured without breaking this caller — which is why the calculator broke three times in six months while nothing about shipping rules changed (Exposing Too Much).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A type named Options, Config, Params or Args, every field optional, growing by a field or two each quarter, with a comment or a wiki page explaining which combinations are valid.
what it might indicate The parameters were bundled to reduce a count rather than because they mean something together. Because the type names no concept, it has no basis on which to refuse a new field, so it accumulates — and because every field is optional, the rules relating them have to live as runtime checks inside the function, where no caller can see them (Invariant Leaks).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff Pull requests whose entire justification is a catalogue name. "Extracted method — the function was long." "Introduced parameter object — five arguments." No requirement is mentioned, and the diff touches code nobody has edited in months.
what it might indicate The team has adopted the vocabulary without the second step. The shapes are being read as defects rather than as prompts, so cleanup effort is spread evenly over a codebase where change is not spread evenly at all.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A type whose public API runs to dozens of methods; a constructor or import list touching every subsystem; a file that appears in the diff of most pull requests regardless of what the pull request is about; a test file measured in minutes.
what it might indicate Responsibilities accumulated where the data already was. The module is now the coordination point for several teams, and its regression surface is the union of everything it does, so every change is priced against all of it.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A commit implementing one sentence of business change, touching modules whose names have nothing to do with that sentence. Reviewers from four teams. A pull request description that has to explain the same rule several times because each module states it differently.
what it might indicate A concept in the business has no corresponding owner in the code, so every module that needed it learned it. The number of edits per rule change now grows with the number of consumers of the concept.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff One file in the diff of most pull requests, whatever they are about. Its test suite covers unrelated subjects. Its imports include a template engine, a payment SDK and a CSV writer. Its change log reads like the product roadmap rather than like a component.
what it might indicate Concerns with different rates of change and different risk profiles share a unit of deployment, review and regression. Every concern is now priced against the union of all of them.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A method that reads five or six members of another object and barely touches its own. Long chains of accessors — order.customer.address.country — and conditional logic written in terms of another module's field values rather than its concepts.
what it might indicate The meaning of that data is being reconstructed outside its owner. The reaching module now depends on the envied type's internal shape, and the same reconstruction is probably happening somewhere else too.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff string email, string currency, int cents, string userId and string orderId in the same signature, unit information carried in variable names, and validation of the same field repeated in five call sites.
what it might indicate A concept in the domain has no representation in the code, so its rules are enforced by convention. Confusable values are interchangeable to the compiler, and the meaning of a value depends on where you found it.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff Signatures with seven or more arguments; several arguments of the same primitive type in a row; two or more booleans; the same four arguments appearing together in several unrelated functions.
what it might indicate Either a group of values that belongs together and has no name, or a function doing two jobs selected by a flag. The repeated clump across signatures is the strong signal; the count on its own is the weak one.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A file or package named utils, helpers, common, shared or misc. Imported by nearly everything. Its test file covers unrelated subjects. Its git history has contributors from every team. Functions inside it use domain nouns — formatOrderRef, isEligibleCustomer — that appear nowhere else in the file.
what it might indicate Concepts the team has not named are being parked. Because the module is at the bottom of the dependency graph, anything domain-shaped that lands there has been stripped of its rules to avoid a cycle, and its contract with nine importers is undocumented.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff The same business rule expressed in a SQL WHERE, a validator, a report and a UI badge — usually in different words, sometimes in different languages. Or, in the false-positive direction, two identical helper functions that a similarity tool has flagged.
what it might indicate A decision has no single home. Every consumer reconstructed it, so they will drift, and the drift will be discovered by a user noticing a contradiction rather than by a test.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff Any ticket touching billing/ comes back as "two to three weeks, maybe". Changes to it are batched into a single quarterly release. The pull requests are reviewed by whoever is available, because nobody claims to understand it, and review comments are about style rather than behaviour.
what it might indicate Behaviour is unknown and unprotected, so the whole cost of the change is discovery and verification. The estimate is not padding — it is an honest price for reading the module, guessing at its assumptions, and hoping production agrees.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A boolean at 100% rollout for eleven months. The off branch still compiles, is still in the test suite, and was last executed in production the day before the rollout completed. Nobody knows who owns it, and its ticket is closed.
what it might indicate The migration finished and its scaffolding was never removed. Every change in this area is still priced against two branches, and the rollback the flag supposedly provides has not been exercised since it stopped being needed.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff An interface with exactly one implementation and no test double. A plugin registry with no plugins outside the repository. An event bus whose only publisher and only subscriber are in the same module. A factory that always constructs the same class. Four layers in which a request object is renamed and passed along unchanged. Type parameters that are only ever instantiated at one type.
what it might indicate The structure was built for a variation that was anticipated rather than observed, and the variation did not arrive. Its cost — indirection on every read, ceremony on every change, and a shape that new code copies — is being paid continuously against a benefit of zero.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A function whose entire body is a field-for-field copy between two types with the same field names and the same field types, and which is edited in the same commit as both of its neighbours, every time.
what it might indicate A boundary was declared where there is no difference in reason to change. The crossing was created to satisfy a diagram rather than to protect anything, and it now multiplies the cost of the most common change in the system (Change Amplification).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A top-level directory named common/, shared/, utils/, helpers/, core/ or lib/, imported by every module, containing files whose names are categories rather than concepts — helpers.ts, utils.ts, misc.ts — and at least one function that mentions a domain noun. Its git history is almost entirely additions by many different authors and almost no deletions.
what it might indicate Ownership is unclear somewhere specific, and this folder is where the ambiguity is being deposited. Each file in it is a concept that was never named, usually because two plausible owners each had a good reason to refuse it. The fan-in makes it the least changeable code in the system, and the absence of an owner means it is never improved, only extended (Afferent and Efferent Coupling).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A plan of pull requests titled "part 1: schema", "part 2: service layer", "part 3: API", "part 4: UI" — described in the standup as a vertical slice plan because the work was broken up.
what it might indicate No increment is releasable, all the risk is in part four, and the first real feedback arrives after the schema is expensive to change. The status board shows 75% complete with 0% delivered.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff Lines that name positions in the code rather than facts about the data: entering pause(), here 3, about to save, saved. Frequently at DEBUG, frequently with an object dumped whole, frequently with no entity id on the line.
what it might indicate The author was debugging without a debugger and committed the scaffolding. The lines answer "which branch ran", which the source already answers, and not "what did it decide about this record", which nothing answers. Volume scales with traffic and usefulness does not.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff An interface with exactly one production implementation, whose only other implementer is a test double; a constructor parameter that is never substituted outside tests; a method made public with a comment saying // visible for testing.
what it might indicate The seam was cut to satisfy a test runner rather than to contain a change. The abstraction has no second case, so it encodes no variation and teaches the reader nothing about the domain (What an Abstraction Actually Is).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff Every attempt at a universal statement needs an exception clause, or the only "property" anyone can write recomputes the expected value using the same logic as the implementation.
what it might indicate Either the behaviour is genuinely a table of decisions with no underlying rule — which is common and fine — or the code has conflated several rules that each have a clean property, and the conflation is why no statement covers the whole thing (Separation of Concerns).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A global client policy of three attempts on any 5xx or timeout, applied to every operation; nested retries at the gateway, the service and the worker; no budget, no jitter, and a dashboard showing the error rate improved when it was introduced.
what it might indicate Retries are being used to paper over a dependency that is chronically degraded, so the underlying problem is invisible and the load multiplier is armed. Under a real outage the dependency receives several times its normal traffic at its weakest moment, and each layer's retries multiply the next (Cascading Failure: When the Response to Failure Causes More Failure in Distributed Systems).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A private cache, lastComputed, hitCount or lazily-initialised field on a type that is constructed once and shared — often added in a commit whose message mentions performance, with no change to any public signature.
what it might indicate The type's concurrency contract has changed and nothing recorded it. An immutable type has become conditionally safe; a thread-safe one may now have an unsynchronised path. Lazy initialisation in particular is a classic: two threads both see the field empty and both compute, which is benign for a pure value and corrupting for anything else (Initialization Races in Concurrency).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff One type holding every capability in the system, taken as the first parameter of most functions — often with a comment saying it keeps signatures short.
what it might indicate Ambient authority has been reintroduced under a new name. Reach is invisible again, the blast radius is the whole bag, and the composition root no longer tells you what any module can do.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff customer.invoices, order.items.length, findAll(), getUsers() — expressions with no visible parameters that reach a store, a service or a file system.
what it might indicate The decision about how much work to do has been taken away from the only party that knows the answer. Expect it to be called in a loop somewhere, and expect that loop to look completely innocuous (N+1 as a Design Problem).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A function whose documentation contains warnings rather than parameters: "caller must ensure the list is sorted", "does not handle empty input", "escape the values before passing them". Grep the call sites and each one has a small block of preparation before the call, and they are not the same block.
what it might indicate The module drew its boundary to protect its own simplicity rather than to hide a decision. The cases did not go away; they went from one place to five, which is the direction that makes a change expensive (Change Amplification).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff Records whose Options section lists one real option and two that were never seriously considered; a Why field that restates the Decision in different words; dates that cluster on the day before an architecture review; and no record at all for the two most-argued choices in the codebase.
what it might indicate The practice is being performed for an audience rather than used. Records are being reverse-engineered from merged code, which means they contain only what the code already says and none of what was believed at the time.
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff Domain behaviour in beforeSave, afterCommit, onSerialize or a request interceptor: a discount applied in an entity callback, an email sent after a transaction commits, a permission enforced in middleware and nowhere else. The domain code reads as though none of it happens, because from the domain's point of view none of it does.
what it might indicate The rule was placed where the data happened to be available rather than where it belongs. It is now invisible to anyone reading the domain, untestable without the framework, and scheduled for renegotiation at the next major version (Hidden Global State).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A third-party type appears in the parameters and return types of your own domain functions — a library's Money, Decimal, DateTime, Result or Schema object crossing module boundaries that have nothing to do with that library's job. Grep for the package name and it hits sixty files across every layer.
what it might indicate The adoption decision was made once, narrowly, and then extended by drift. Nobody has priced the exit, and the exit is now the size of the codebase. It also means your modules are coupled to each other *through* the vendor type: change how you represent money and you are negotiating with a library's release schedule (Primitive Obsession with someone else's primitive).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A top-level folder called utils/, common/, shared/, helpers/ or lib/ that is among the most-imported in the repository, has no owner, contains files named misc.ts or index.ts with forty unrelated exports, and whose contents range from a string-padding function to the tax calculation.
what it might indicate The structure has no home for shared mechanism, so everything shared lands in one place regardless of what it is. The consequence is not untidiness: it is that domain knowledge is now stored in a folder with no owner and no cohesion, so a change to the tax rule is a change to a file that half the codebase imports, and the blast radius of every edit there is the whole repository (God Object at directory scale).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff Every RFC is accepted. The Alternatives section describes each rejected option in one dismissive sentence. Non-Goals says "none" or is missing. Comments are about wording, formatting and section completeness rather than about the choice. The implementation was underway before circulation, and the RFC's merge commit is later than the first implementation commit.
what it might indicate The process has become a publication channel with an approval ritual attached. Its real function is legitimising decisions rather than making them, which is worse than having no process: it consumes reviewer attention, it teaches everyone that objecting is pointless, and it produces a false record that a future engineer will read as evidence that alternatives were seriously weighed (Documentation Decay in its most expensive form, because the document is not merely stale but was never true).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.
what you see in the diff A prompt containing sentences of the form "if the customer is X, do Y", "never exceed N", "only for accounts on the Z plan" — thresholds, tiers, windows, entitlements and state transitions written in English inside a string that no test asserts against.
what it might indicate A business rule with a crisp definition has been delegated to a component that cannot guarantee it. The rule is now untestable (no assertion can be written), unenforceable (output is sampled), unauditable (no record of which rule fired), and silently mutable (a wording edit or a model upgrade changes it with no diff that looks behavioural).
Say out loud what you would write in the review — and whether you would raise it at all. Then open the rest.