Seven Review Warning Signs That Predict Expensive Future Changes

Most problematic code does not look broken. It compiles, passes its tests and behaves well in a demo, yet experienced reviewers can often tell within a minute that the next change, the next incident or the next handoff is going to hurt. This guide walks through seven of the signals they react to, explains what each one predicts, and shows how to tell a genuine problem from a pattern that merely looks suspicious, so you can apply the same judgment to your own reviews.

What a code smell actually tells you

A seasoned reviewer rarely needs to read a whole module to sense trouble. A boolean argument whose purpose is invisible where the function is called, a utility that has grown a dozen optional behaviors, a catch that turns every failure into the same innocent-looking value: none of these break anything today. The problem is what they will demand tomorrow.

The instinct comes from experience rather than from a stricter style guide. People who have maintained systems for years have watched one copied rule turn into three contradictory versions, a handy abstraction become the place where every special case lands, and a default fallback mask a production outage for weeks. What they have learned to see is where a piece of code collects uncertainty.

It helps to treat a smell as a prompt, not a verdict. It does not prove the implementation is wrong; it says the code has earned closer scrutiny:

• Which person or module is responsible for this decision?
• How does this hold up once the requirement shifts?
• Will a failure still be visible when it occurs?
• Could the next developer understand why the code is shaped this way without tracking down whoever wrote it?

The seven signals below are the ones that most reliably turn into cost if nobody asks those questions early.

1. One function doing several unrelated jobs

Long functions attract criticism, but length is seldom what worries an experienced reviewer. A function can run to many lines because it carries out one clear operation in a sequence of readable steps. Another can fit in twenty lines and still be hard to work with because it handles input validation, permission checks, saving to the database, shaping the response, triggering notifications and translating errors all at once.

The real issue is that mixed responsibilities give a function many independent reasons to change. A new access rule touches it. A change to the response format touches it. A schema migration touches it. A new notification requirement touches it. Concerns that ought to evolve separately end up meeting in one place, and even a minor edit forces you to understand the entire flow.

A useful tell is a name that cannot describe the function honestly. Something called processOrder might check the customer, compute prices, hold stock, create a payment intent, persist the order and email a confirmation. The name stays vague because an accurate one would read like a paragraph.

Why mechanical extraction is not the fix

Splitting the function into smaller pieces does not automatically help. Pulling out chunks by line count can scatter the same confusion across more files while every hidden dependency survives. A better approach is to look for boundaries that mean something:

• Where is a business decision being made?
• Which part is a side effect on an external system?
• Which part is pure data transformation?
• Which part owns the transaction?

Once those seams are visible, the code can be organized the way the operation actually works. The underlying problem was never the amount of work the function performs. It was that nobody could change a single decision without holding several unrelated ones in mind at the same time.

2. Boolean parameters that switch on hidden modes

A boolean argument looks harmless because it carries a single bit. In practice it frequently smuggles a whole alternative workflow in under an existing function name. Consider a call like the one below, which tells a reader almost nothing about what will happen:

await updateAccount(accountId, true, false);

To understand it, you have to open the implementation and find out whether the first true forces the update, triggers a notification, bypasses validation, keeps history or activates the account. Whoever wrote the function may remember, but everyone else has to reconstruct the meaning from argument positions, and a swapped pair of literals will pass every type check.

Combinations nobody designed

The problem compounds as flags accumulate. Three booleans produce eight possible combinations, several of which nobody ever thought about. If a function accepts forceUpdate, skipValidation and sendNotification, what is supposed to happen when a forced update skips validation but still notifies someone? The type system happily allows that call even if the business domain should not.

Reviewers flag this because a set of flags usually means one function is standing in for several operations. They may share internals, but they do not share intent. Editing contact details, verifying an account and applying an administrative override might all write to the same row, yet no caller should need to remember which flag pattern selects which workflow.

Choosing the right refactor

In some cases, switching to a single argument object whose properties have names is sufficient, because updateAccount(id, { notify: true }) at least explains itself at the call site. In other cases separate functions, such as verifyAccount and overrideAccount, make the business intent explicit and remove impossible combinations entirely. Which route fits depends on whether the behaviors are real variations of one operation or distinct operations that happen to share code.

Booleans are not inherently bad. A plainly named option like includeArchived is perfectly reasonable. The warning sign is a flag that changes what the function means rather than tweaking one well-understood detail.

3. The same business rule expressed in different forms

Copy-pasted text is easy to spot. Duplicated knowledge is more dangerous, because each copy can look entirely different.

Take a permission rule. It might appear as a hidden button in the UI, a role check in a controller, a status condition in a service and a WHERE clause in a query. Each layer seems to be protecting the system, but each may encode a slightly different idea of who is permitted to act.

Everything holds together right up until the policy changes. One endpoint picks up the new rule, another keeps the old logic, and a scheduled job sidesteps both by going straight to an internal service. The interface now offers an action the server refuses. One part of the application treats a user as authorized because of their role, while another insists they must own the record.

Experienced reviewers notice recurring combinations of statuses, roles, flags and dates, because such conditions often represent the same business meaning written more than once. They ask where the authoritative decision lives, and whether the other layers enforce that decision or quietly redefine it.

Layers with different purposes are fine

This does not mean a rule may appear in only one line of code across the whole system. The UI may reflect authorization to improve usability, while the backend must enforce it for security. A database constraint may guard the same invariant at the storage level. Those layers have distinct jobs. The smell is present when each one answers the business question on its own terms.

A healthy design gives the rule a single deliberate owner and lets the other parts consume, enforce or display the outcome. When ownership is unclear, every future policy change turns into a search exercise: track down each place the rule is quietly encoded and trust that you found them all. Anyone who has lived through policy drift learns that repeated knowledge usually costs more than repeated code.

4. A shared helper with a growing options object

Many troublesome abstractions start life as sensible helpers. Two places repeat similar logic, the common part is extracted, and the result is small, focused and easy to follow.

Then the two callers start to want different things. One needs retries; the other must never repeat its request. One benefits from caching; the other must never see stale values. One hands back a complete payload; another streams. Rather than splitting those workflows, the helper sprouts a new option for each difference.

Before long the options object covers error translation, result reshaping, caching knobs, retry counts, auth handling, hook functions and a handful of boolean switches. Most callers use two or three of these, but any change risks affecting all of them.

This is a smell because the abstraction has stopped removing complexity and started concentrating it. Call sites look short, but understanding the system now means knowing how every option interacts with every other. A large number of callers is not evidence that the abstraction is sound; sometimes it only shows that the helper has become impossible to avoid.

Similar syntax versus shared behavior

The key question is whether the callers share stable behavior or merely similar-looking code. Two operations may both issue HTTP requests yet treat failures and retries in opposite ways. A pair of forms might have the same fields but different validation, submission and draft handling. Merging them removes visible duplication while making every real difference harder to express.

What works better is usually a narrower abstraction introduced later. Instead of sharing the whole workflow, share a single stable piece, such as one transformation, one policy or one low-level call, and keep the distinct workflows explicit. Reuse is still valuable; what should raise concern is a pattern where each new requirement adds yet another option that existing callers neither need nor understand. For a frontend-focused view of the same problem, see why frontend abstractions quietly turn into technical debt.

5. Error handling that disguises failure as success

A catch block can make an application look sturdy while quietly eroding confidence in it.

Picture a data-access layer that responds to a database error by handing back []. Or a service whose HTTP call fails and which simply returns null. A frontend maps every rejected request to an empty state. Nothing crashes, but the system can no longer tell a legitimate absence of data from work that failed.

Reviewers catch this one quickly because they know how it plays out during an incident. A dashboard reports zero records while the database is down. A customer sees "No orders found" after their session silently expired. A background job marks an item as done because a dependency returned nothing usable. The interface stays calm because it has swapped the truth for a believable answer.

Questions to ask of every catch block

When reviewing error handling, check three things:

• Is the original cause preserved, for example as the cause of a new error, or at least in the logs?
• Can the caller distinguish an expected business outcome from a technical failure?
• Does the layer that catches the error actually own the next decision?

When the code at that level has no way to recover, try again, undo partial work or provide a sensible substitute, catching the error may do nothing except destroy evidence.

None of this means raw exceptions should reach users. Messages shown to people should be safe and comprehensible, and internal errors often need translating into stable domain categories. The requirement is that the translation keeps enough meaning for both the application and the developers to respond correctly.

An empty result should mean the operation succeeded and found nothing. A failed operation should stay a failure unless the system has deliberately defined a safe fallback. Merging the two for convenience slows debugging and shows users misleading information. Good error handling contains a failure; bad error handling hides it.

6. A small requirement with a wide, unrelated diff

Experienced reviewers often look at the shape of a pull request before reading individual lines. When a modest requirement spreads across a long list of unrelated files, that usually points to hidden coupling or to a task that has drifted from its goal.

Adding a single field might legitimately reach persistence, services, API responses and the UI; that is a coherent change across layers. The smell appears when the same pull request also rewrites shared utilities, renames unrelated types, alters error handling, moves modules around and refreshes large snapshot files.

Each of those edits might be an improvement, but together they bundle several reasons for change into one unit. Reviewers must assess the feature, the refactor, the contract change and the cleanup simultaneously. If a regression slips through, finding the responsible edit is harder. If the feature has to be reverted, the unrelated improvements go with it.

Why AI-assisted coding makes this more common

Code generation tools make this pattern easier to fall into, because complete rewrites cost almost nothing to produce. Ask for a single bug fix and you may receive a polished replacement function with new names, different control flow, fresh abstractions and changed error behavior. Producing it was nearly free; verifying it still costs the team the same review effort as ever.

A good rule is to keep the size of a change in proportion to how clearly it can be explained. A narrow diagnosis usually deserves a narrow fix, and broader cleanup can go into its own pull request, where it can be reviewed and tested on its own merits. That is not a demand for tiny pull requests everywhere; some architectural work is necessarily broad. The question is whether every meaningful edit serves one coherent outcome. When the diff is bigger than the problem, several concerns are usually being handled without being separated, and cause and effect become harder to trace.

7. Code that works but resists verification

Some implementations get written quickly precisely because their behavior is hard to observe or control. A function reads the system clock directly, constructs its own dependencies, mutates global state, makes several external calls, swallows its own errors and reports almost nothing back. The success case might be fine, but demonstrating how it fails becomes expensive.

Testability tends to reflect the quality of boundaries. If a business rule cannot be exercised without booting the whole application, seeding database rows, calling real services and waiting on real timers, the rule is probably entangled with too many side effects.

Asynchronous workflows expose the gap

The problem is sharpest in multi-step async flows. Imagine a function that saves a change, emits a domain event, notifies a user and appends an audit log row. If the third step fails, can a test tell which steps already completed? Can you force a dependency to time out? Can you simulate the function running twice? Can you reproduce a stale update without relying on timing luck?

Code that is easy to write but hard to verify pushes confidence into manual testing. Someone clicks through the successful path, sees the right result and concludes the feature is done. Production then brings concurrency, retries, odd data and half-finished operations that nobody exercised locally.

Verify at the level where the risk lives

Purity everywhere and mocks for everything are not the answer either. Tests built mostly on mocks tend to freeze internal details in place while overlooking how the actual database, network or transactions behave. The better approach is to decide which guarantees matter and check each one where its risk actually sits:

• Pure transformations can be unit-tested in isolation.
• Database invariants often need integration tests against a real database.
• Service boundaries may call for contract tests.
• Distributed behavior may depend on structured logs and correlation IDs, because not every production condition can be reproduced faithfully.

Injecting a clock or a dependency, instead of reaching for globals, is often the smallest change that makes the important decisions controllable. The smell is not low coverage as such. It is code whose key decisions cannot be controlled, observed or explained once the happy path stops cooperating.

Key takeaways

None of these signals is an automatic verdict. A long function may represent a single clear operation, a boolean option may be entirely obvious, a shared helper may capture genuinely stable behavior, and a broad change may be unavoidable. Treat the list as a way to decide where to dig deeper, not as a checklist to match mechanically.

When you review, the underlying questions are:

• Are responsibilities mixed, so one change drags unrelated decisions along with it?
• Can callers understand a function without decoding positional flags?
• Does each business rule have one clear owner?
• Is a shared abstraction hiding real differences behind options?
• Does error handling preserve the truth about what failed?
• Is the diff proportional to the requirement it serves?
• Can the important behavior be verified when things go wrong?

What experienced reviewers are really detecting is deferred uncertainty: the chance that the next requirement will force someone to understand unrelated logic, that the next incident will leave too little evidence to investigate, that the next developer will not know which behavior was intentional, or that the next caller will pile yet another option onto an abstraction that already carries too much. They recognize these smells quickly because they have already paid for them, untangling overloaded helpers, tracing swallowed exceptions, reconciling contradictory rules and reviewing the "small" feature that touched half the codebase. The goal is not to eliminate code that looks untidy; it is to catch code that will make future work harder than it needs to be.