code Review/antipatterns/Structural Antipatterns: Hidden Side Effects, Temporal Coupling, and God Objects

antipatterns

Structural Antipatterns: Hidden Side Effects, Temporal Coupling, and God Objects

Five code shapes that reliably produce future defects — and how to raise them in review with a named consequence instead of a matter of taste.

Reviewer Detection Checklist

0 of 12 verified (0%)

Defect Patterns & Fixes

A read-shaped method that writes

java

Scenario: A profile page renders a user's notification preferences.

public Preferences getPreferences(String userId) {
    Preferences prefs = repo.find(userId);
    if (prefs == null) {
        prefs = Preferences.defaults(userId);
        repo.save(prefs);              // a write, behind a getter
        auditLog.record("preferences.created", userId);
    }
    return prefs;
}

// In the view layer, called three times while rendering one page.

Why this is defective: Callers treat a get as free and idempotent, so it ends up in loops, templates, and health checks. Here that means a write and an audit event on a page render — three of them if the template touches preferences three times, and a duplicate-key error the moment two requests for a new user arrive together. The write also runs outside any transaction the caller may have opened, so it survives a rollback that was supposed to undo the whole request. Splitting the method removes an entire family of possible defects rather than patching one.

How to Spot It in Reviews:

•Read the verb in the name, then look for save, insert, publish, send, or a client call in the body.
•Ask how many times a page render or a request calls this method — nobody counts calls to a getter.
•Lazy creation inside a read is also a concurrency finding: two simultaneous first-time reads race to create.
•Check whether the hidden write participates in the caller's transaction; usually nobody has considered it.

Temporal coupling through mutable request context

csharp

Scenario: A multi-tenant reporting service scopes its queries with a tenant set on the service instance.

public class ReportService            // registered as a singleton
{
    private string _tenantId;

    public void SetTenant(string tenantId) => _tenantId = tenantId;

    public Report Build(DateRange range)
    {
        // Silently uses whatever tenant was set last — possibly another request's.
        return _repo.Query(_tenantId, range);
    }
}

Why this is defective: The API has an invisible precondition: Build is only correct if SetTenant was called first, on this instance, for this request. Forget it and the report is built for the previous caller's tenant — a cross-tenant data leak that returns a perfectly well-formed, plausible report, so nothing alerts and the customer is the detector. On a singleton it is also a data race: two concurrent requests interleave the set and the build. Passing the tenant as a parameter makes the precondition impossible to omit and deletes the shared state at the same time.

How to Spot It in Reviews:

•A setter and a doer on the same object, where the doer reads what the setter wrote.
•Check the DI lifetime: singleton or scoped changes this from 'confusing' to 'cross-request data leak'.
•Ask what Build does if SetTenant was never called — a null or a stale value is worse than an exception.
•Any field that is really 'the current request's X' is a parameter wearing a disguise.

Boolean trap and transposable primitives

python

Scenario: A transfer API is extended with two behaviour switches.

def transfer(source: str, target: str, amount: float,
             immediate: bool, notify: bool) -> None:
    ...


# At the call site, months later:
transfer(target_account, source_account, 250.0, False, True)

Why this is defective: Two adjacent str parameters mean source and target can be swapped with no type error, no test failure, and no review signal — the money simply moves the wrong way. Two adjacent booleans mean the call site reads False, True and cannot be understood without opening the definition, so the next change gets them backwards. Distinct types for accounts and money make transposition a compile-time or construction-time error, and the enum makes the call site self-describing. Money carrying its currency also closes the float arithmetic hole in the same move.

How to Spot It in Reviews:

•Scan call sites, not signatures: bare literals and same-typed adjacent arguments are the tell.
•Two or more booleans in one signature usually encode a small state machine — ask what the four combinations mean.
•float for an amount and str for an identifier are both primitive obsession with a known failure mode.
•Keyword-only arguments are a cheap partial fix in Python and are worth suggesting when a value type is too big a change.

Repository leaking persistence types to its caller

Scenario: A handler serialises a list of orders returned straight from the data layer.

// storage layer
func (r *OrderRepo) Recent(ctx context.Context) ([]*sqlx.Row, error) { ... }

// http layer
func (h *Handler) List(w http.ResponseWriter, req *http.Request) {
    rows, _ := h.repo.Recent(req.Context())
    json.NewEncoder(w).Encode(rows)     // database shape becomes the API contract
}

Why this is defective: When the persistence shape is the response shape, a schema change becomes an API change: renaming a column breaks every client, and a column added for internal bookkeeping is published to the world — which is how internal cost fields and soft-delete flags end up in public responses. The same leak in an ORM is worse, because the returned entity carries lazy proxies that issue queries during serialisation, turning a JSON encode into an N+1 far from any code that looks like data access. An explicit type at the boundary decouples the two and makes the contract reviewable.

How to Spot It in Reviews:

•A framework or driver type in an exported signature: sqlx.Row, a JPA entity, a DbSet, an HttpResponse.
•Serialisation of a value that came directly out of the data layer with no mapping step.
•Ask what happens to the API when a column is renamed — if the answer is 'clients break', the abstraction leaks.
•In ORMs, check for lazy collections crossing the boundary; the performance defect and the coupling defect are the same line.

Why structural antipatterns count as defects

A structural antipattern is not a bug today. It is a shape that reliably produces bugs, and it is the only defect class where the cost of catching it late is unbounded — because by then every new call site has been written against the bad shape.

The distinction that matters in review is between taste and consequence. "I would have used a different name" is taste. "This getter opens a database connection, so calling it twice in a template doubles the query count and it cannot be used in a test without a database" is a consequence, and consequences belong in a review. An antipattern finding is only worth writing down when you can name the defect it will cause.

The five shapes below are the ones that pay for themselves. Each has a specific downstream failure, and each is visible in a diff.

The shapes and what they cost in production

1. Hidden side effects. A method whose name promises a read and whose body performs a write, a network call, or a mutation. getUser() that creates a user, isValid() that saves, a property getter that lazily initialises via I/O. The cost: callers reason about the code as if it were free and idempotent, so they call it in loops, in templates, in logs, and in retries. Duplicate writes and N+1 queries both originate here.

2. Temporal coupling. Two methods that must be called in a specific order, with nothing enforcing it. configure() then start(); setTenant() then query(). The cost: the second caller — often months later, often in a new thread — omits the first step and gets a null, a default, or the previous caller's tenant. Cross-tenant data leaks frequently trace back to a setTenant-shaped API.

3. Global mutable state. Singletons with setters, module-level configuration objects, static caches, thread-locals used as implicit parameters. The cost: tests become order-dependent and flaky; concurrency defects become possible; and the actual dependency graph of the system is invisible, so nobody can reason about what a change affects.

4. Boolean traps and primitive obsession. transfer(account, amount, true, false). Ids, money, and durations passed as bare string, double, and int. The cost: argument transposition that the compiler cannot catch. Two String parameters in the wrong order is a defect that type-checks, passes review, and moves money to the wrong account.

5. Leaky abstractions and god objects. A repository that returns ORM entities with lazy proxies attached, a UserService with forty methods, a "manager" that owns unrelated concerns. The cost: changes ripple. Every feature touches the same file, merge conflicts become constant, and the blast radius of any change is the whole class.

How to spot it in review

Antipatterns are easier to find by asking questions about the interface than by reading the implementation.

Read the method name, then the body. Does the body do more than the name promises? Any I/O, mutation, or lazy initialisation behind a read-shaped name is a finding.
Look for the second required call. If the diff adds an init, configure, setContext, begin, or attach, ask what happens when someone calls the main method without it. If the answer is "wrong data" rather than "clear error", that is the defect.
Count the constructor arguments and the fields. A class taking eight collaborators is doing eight things. It is also, in practice, untestable without a container.
Find the literal true/false/null at call sites. A boolean argument is unreadable at the call site by construction. Two or more is a state machine that should be separate methods or an enum.
Check what crosses the boundary. If a service returns a database entity, an HTTP response object, or a framework type to its caller, the abstraction leaks and the caller is now coupled to the transport or the schema.
Ask how it is tested. "You would need a database and a real clock" is a design finding, not a testing complaint. Difficulty of testing is the most reliable early signal of a structural problem.

Note

A useful review question for any new interface: "what is the worst thing a reasonable caller could do with this, by accident?" Good interfaces make the accident impossible; antipatterns make it easy and silent.

Fixing it without breaking something else

Structural changes have the widest blast radius of any fix, so the sequencing matters more than the destination.

Splitting a god class is a mechanical refactor with a behavioural risk: shared private state becomes shared public state unless it moves too. Move the state and the behaviour together, or you have created coupling in a new place.
Removing a global changes lifetime and identity. Code that relied on "the same instance everywhere" — a cache, a counter, a connection — behaves differently when it becomes per-request. Verify what depended on the sharing before you delete it.
Changing a return type from an entity to a DTO surfaces lazy-loading errors that the leak was hiding. That is the point, but it means the change is bigger than the signature suggests.
Introducing a value type touches every call site. Do it in one direction at a time: add the type, accept both, migrate callers, remove the primitive overload.
Making an implicit ordering explicit can break callers who never called init and were accidentally relying on a default. Find them first.
Do not mix a structural change with a behaviour change in one PR. If a refactor and a fix land together, a bisect cannot separate them and the reviewer cannot verify either.

How not to over-engineer the fix

The failure mode of antipattern review is trading one shape for a more fashionable one. The bar: does the change remove a class of defect, or does it just move the code?

Proportionate responses:

Boolean trap → two named methods, or one enum parameter. Not a builder, not a command object.
Hidden side effect → rename it, or split it in two. loadOrCreateUser() is a complete fix. A CQRS split is not required to make a method honest.
Temporal coupling → make the first call return the thing the second needs. A type that cannot exist until configure() has run enforces the order for free, with no framework.
Global state → pass it as a parameter. Constructor injection of one collaborator beats a service locator, an ambient context, or a DI container feature nobody else uses.
God class → extract the one cohesive piece that has its own reason to change. One extraction per PR. Splitting forty methods into eight classes in one change is unreviewable, and unreviewable refactors are where new bugs hide.

Push back on these in review: interfaces created for a single implementation with no second one in sight; a hexagonal/ports-and-adapters rewrite proposed in response to one leaky method; a mapper layer added between two identical shapes; and "let us extract a base class" when the two things share code but not a concept.

Important

Antipattern findings compete for the author's attention with correctness findings. If you have three structural observations and one race condition, lead with the race condition and mark the rest as non-blocking. A review where everything is important is a review where nothing is.

Comment templates that an agent can act on

"getPreferences() writes a default row when none exists — the name promises a read, so callers invoke it in a loop. Rename to loadOrCreateDefaults() and move the write out of the render path."
"transfer(from, to, true, false) — the two booleans are unreadable at the call site. Replace with TransferOptions or split into transferImmediate / transferScheduled."
"setTenant() must be called before query() or the previous request's tenant is used. Take the tenant as a parameter of query() instead."
"This repository returns the JPA entity, so lazy collections are loaded in the controller. Return a DTO projected in the query."
"OrderManager now has 41 methods covering pricing, fulfilment, and notification. Extract pricing — it has its own reason to change — and leave the rest for a follow-up."

Previous Guide

N+1 Queries, Unbounded Memory, and Hot Loops: Spotting Performance Defects in Review

Next Guide

Over-Engineering and Premature Abstraction: The Defects Reviewers Under-Report

Interactive Practice

Review Defective Code in the Workbench

Test your ability to spot this defect in our interactive Monaco-powered PR code editor.

Open Workbench