Over-Engineering and Premature Abstraction: The Defects Reviewers Under-Report
Speculative interfaces, unused configuration, forwarding layers, and infrastructure added with no measurement — how to spot complexity that is not paying for itself.
Reviewer Detection Checklist
Defect Patterns & Fixes
Strategy, factory, and registry for a single case
public interface DiscountStrategy { BigDecimal apply(BigDecimal amount); }
public class PercentageDiscountStrategy implements DiscountStrategy { ... }
public class DiscountStrategyFactory {
private final Map<String, DiscountStrategy> registry = new HashMap<>();
public DiscountStrategyFactory() {
registry.put("PERCENTAGE", new PercentageDiscountStrategy());
}
public DiscountStrategy forCode(String code) {
return registry.getOrDefault(code, new NoOpDiscountStrategy());
}
}
// One call site:
BigDecimal total = factory.forCode(order.getDiscountCode()).apply(subtotal);How to Spot It in Reviews:
- •Count implementations of every new interface in the PR. One is a question; one plus a factory is a finding.
- •Look for a permissive default (getOrDefault, orElse(noOp)) that converts an unknown input into silence.
- •Check whether the set of supported values is still visible to a reader after the change.
- •Ask when the second implementation is due. 'Eventually' means build it eventually.
Configuration for values nothing varies
RETRY_ATTEMPTS = int(os.getenv("RETRY_ATTEMPTS", "3"))
RETRY_BASE_DELAY = float(os.getenv("RETRY_BASE_DELAY", "0.5"))
RETRY_MULTIPLIER = float(os.getenv("RETRY_MULTIPLIER", "2.0"))
RETRY_JITTER = float(os.getenv("RETRY_JITTER", "0.1"))
RETRY_STRATEGY = os.getenv("RETRY_STRATEGY", "exponential") # or "linear", "fixed"
RETRY_ON_STATUSES = os.getenv("RETRY_ON_STATUSES", "500,502,503,504").split(",")How to Spot It in Reviews:
- •Check the deployment configuration for whether any environment overrides the default. If none does, it is a constant.
- •A configuration value that selects between code paths multiplies the tested surface; treat it differently from a numeric tunable.
- •Ask who changes this, when, and how they would know what to set it to.
- •String-typed enums parsed from the environment fail at runtime in the environment that got it wrong, which is production.
Distributed infrastructure for in-process state
// "Fix" for a race on a per-process counter.
func (s *Service) Increment(ctx context.Context, key string) error {
lock, err := s.redis.ObtainLock(ctx, "counter:"+key, 5*time.Second)
if err != nil {
return err
}
defer lock.Release(ctx)
s.counts[key]++ // still a plain map, still in one process
return nil
}How to Spot It in Reviews:
- •Match the fix to the scope of the data. Cross-instance invariants need cross-instance coordination; a local map does not.
- •A new infrastructure dependency introduced by a bug fix deserves the same scrutiny as a new feature.
- •Ask what happens when the new dependency is down — a correctness fix that adds an availability risk is a trade, not a win.
- •If the invariant really is cross-instance, a database unique constraint is usually simpler and more reliable than a lock.
A mapping layer between identical shapes
public record OrderDto(Guid Id, string Status, decimal Total, DateTimeOffset PlacedAt);
public record OrderResponse(Guid Id, string Status, decimal Total, DateTimeOffset PlacedAt);
public static class OrderMapper
{
public static OrderResponse ToResponse(OrderDto dto) =>
new(dto.Id, dto.Status, dto.Total, dto.PlacedAt);
}How to Spot It in Reviews:
- •Compare the two types field by field. Identical means the boundary is nominal.
- •Ask what the API should do when the model gains a field — if the answer is 'add it to both', the layer costs without protecting.
- •Distinguish this from a genuine boundary type: an entity with lazy proxies or internal-only fields does need one.
- •Auto-mappers hide the same problem behind reflection and turn a forgotten field into a silent null rather than a compile error.
Why over-engineering is a defect, not a preference
Reviewers are trained to find missing things. Over-engineering is the opposite failure — something present that should not be — and it is systematically under-reported because objecting to extra rigour feels like arguing for sloppiness.
It is worth reporting because the cost is real and compounding. Every abstraction is a thing that must be understood before anything inside it can be changed. Every configuration point is a combination that is never tested. Every layer is a place a defect can hide and a place a stack trace has to travel through. Speculative generality does not sit inert; it slows every future change and hides the defects that matter behind indirection.
The clearest formulation for a review: complexity is justified by a requirement that exists today, or by a specific cost you can name. Not by one that might exist, and not by "it is more flexible".
What it costs in production
- Slower incident response. During an outage, an engineer traces a request through six layers of indirection to find the one line that does the work. Time-to-diagnosis is a function of how many hops there are.
- Untested combinations. A component with seven configuration flags has 128 configurations and tests for three. The production configuration is often one of the untested ones, and the failure appears only there.
- Defects hidden by indirection. A dynamic dispatch table, a reflective factory, or a rules DSL defeats "find usages". Reviewers cannot see what runs, and neither can static analysis.
- Abstractions that fit the imagined case, not the real one. A plugin interface built for hypothetical providers turns out to have the wrong shape when the second provider actually arrives — so you now maintain the abstraction and work around it.
- Operational surface for no traffic. A queue, a cache, and a worker fleet introduced for a feature with fifty daily calls: three more things to deploy, monitor, secure, upgrade, and page someone about.
- Onboarding cost. Every new engineer pays the cost of understanding the framework you wrote instead of the domain they were hired to work on.
How to spot it in review
Count the concrete implementations. An interface, a strategy, a factory, or a plugin registry with exactly one implementation is speculative until proven otherwise. Ask directly: what is the second one, and is it on the roadmap this quarter?
Count the callers. A parameter, hook, or extension point with one caller that always passes the same value is dead flexibility. So is a generic type parameter that is only ever instantiated with one type.
Look for configuration nobody will change. A value in a YAML file, an environment variable, and a feature flag are three places to look during an incident. If operations will never change it, it is a constant with extra steps.
Look for infrastructure introduced without a number. A cache, a queue, a read replica, or a shard added in a PR that contains no measurement. The question is "what is the current latency, volume, or size, and what will it be?" — not whether the technology is good.
Look for layers that only forward. A service that calls a manager that calls a repository, where two of the three do nothing but pass arguments through. Mapping between two identical shapes is the most common instance.
Look for premature generalisation of a fix. A one-line correctness bug that arrives with a validation framework attached. The fix and the framework should not be in the same PR.
Tip
The rule of three is still the best heuristic available: build the abstraction on the third occurrence, not the first. Two similar things are frequently a coincidence, and the abstraction built from two examples usually has to be rebuilt when the third arrives.
What is not over-engineering
This category is easy to over-apply, and a reviewer who treats every abstraction as speculative does as much damage as one who accepts all of them. These are not over-engineering, and calling them out will cost you credibility:
- Error handling, input validation, and boundary checks for inputs that actually occur.
- A test suite that seems large relative to the change — tests are the cheapest insurance available.
- An interface introduced to make something testable when the alternative is no test at all.
- A value type replacing a primitive that has already caused a defect.
- Idempotency, retries with backoff, and timeouts on anything that crosses a network. Those are requirements, not embellishments.
- Structured logging and metrics on a new code path — Jagopy's own convention is that every new feature emits telemetry, which is a stated requirement rather than gold-plating.
The distinction is always the same: does this respond to a real, present condition, or to an imagined future one?
Fixing it without breaking something else
Removing complexity is still a change, and it deserves the same care as adding it.
- Inlining an abstraction can lose behaviour it quietly provided — a default, a retry, a cache, a null check. Read the whole implementation before collapsing it.
- Deleting a configuration point changes deployed behaviour if any environment set it to a non-default value. Check the deployed configuration, not just the defaults in the repository.
- Simplification PRs should be separate and mechanical. Mixed with a feature, they make review and bisect impossible.
- Do not simplify what you have not read the history of. A layer that looks pointless sometimes exists because of an incident.
git logon the file answers this in thirty seconds. - Beware of relitigating a decision that has already been made. If the team chose an architecture, a single PR review is the wrong venue to reverse it. Raise it separately.
How to raise it without stalling the review
Over-engineering findings are almost never blocking, and framing them as blocking is how reviewers lose the argument. Effective framing:
- Name the cost, not the taste: "three implementations of this interface exist, all in this PR, and all are called from one place" beats "this feels over-abstracted".
- Ask a question with a factual answer: "what is the second implementation this interface is for?"
- Offer the smaller version concretely: "a function taking a
Formatterargument would cover both cases without the registry." - Mark severity honestly. Speculative generality is rarely as urgent as a missing null check, and treating them equally trains authors to discount your comments.
- Accept "yes, and we need it" when the author has a reason you did not have. The goal is a decision made deliberately, not a fight won.
Warning
The most expensive over-engineering in code review is the reviewer's: demanding an abstraction, a policy engine, or a framework in response to a defect that needed three lines. Before asking for structure, ask what the minimum correct change is and whether you can justify anything beyond it.
Comment templates that an agent can act on
- "
PricingStrategyhas one implementation and one call site. InlineStandardPricinginto the caller and delete the interface and the factory." - "This registry resolves handlers reflectively by name, so 'find usages' returns nothing. Use an explicit map from event type to handler."
- "
retry_count,retry_delay, andretry_jitterare configurable but every environment uses the defaults. Make them constants until something needs to vary." - "The Redis cache is added with no measurement. What is the current p95 for this query, and how often is it called? A composite index on (tenant_id, created_at) may remove the need entirely."
- "The mapper between
OrderDtoandOrderResponsecopies eleven identical fields. Use one type until the shapes actually diverge."
Review Defective Code in the Workbench
Test your ability to spot this defect in our interactive Monaco-powered PR code editor.