Design Review: Trade-Offs and Maintainability

Introduction

A design review asks whether an object model will let the team make likely changes safely. It is not a contest to spot the most patterns or cite the most principles. Start with a change request, follow it through the objects that own the behavior, then ask what else could break when the change ships.

This article uses a shipping quote flow to make that review concrete. The same questions apply to pricing, identity, scheduling, persistence, and other boundaries where domain rules meet external systems. For a companion guide to assigning behavior in the first place, see GRASP responsibility assignment and testable object design.

Review the Change, Not the Diagram

Begin with a specific scenario: “A second carrier must support international shipments, while the existing carrier keeps its current rules.” Identify the code expected to change, the rules that must stay intact, and the evidence that will prove the result works. If the scenario is vague, ask the product or operations owner to clarify it before debating class boundaries.

Then trace the behavior from the entry point to the effect:

flowchart LR
    Request[Quote request] --> UseCase[Quote service]
    UseCase --> Policy[Shipping policy]
    Policy --> Carrier[Carrier adapter]
    Carrier --> Provider[Carrier API]
    UseCase --> Store[Quote store]
    UseCase --> Telemetry[Metrics and audit]

This is one possible shape, not a target every system must copy. The review should establish which object validates the request, which owns shipping rules, which translates the carrier protocol, and which records a quote. If one object handles all four jobs, ask whether those responsibilities change together or merely happen to sit next to each other in the current code.

A Before-and-After Design

The initial implementation is short, but it combines request validation, carrier selection, transport, retry behavior, and persistence:

final class QuoteService {
    private final QuoteRepository quotes;
    private final HttpClient http;

    Quote quote(Shipment shipment) {
        if (shipment.weightKg() <= 0) {
            throw new IllegalArgumentException("weight must be positive");
        }
        String url = shipment.isInternational()
                ? "https://intl.carrier.example/quote"
                : "https://domestic.carrier.example/quote";
        HttpResponse response = http.post(url, encode(shipment));
        Quote quote = decode(response);
        quotes.save(quote);
        return quote;
    }
}

Suppose international shipments need a different credential, timeout, response shape, and audit field. The service now has several reasons to change, while tests need an HTTP client and repository even to check a weight rule. A first refactor can give the stable policy and the volatile protocol separate owners:

interface Carrier {
    Quote quote(Shipment shipment);
}

final class QuoteService {
    private final Carrier domestic;
    private final Carrier international;
    private final QuoteRepository quotes;

    Quote quote(Shipment shipment) {
        validate(shipment);
        Carrier carrier = shipment.isInternational() ? international : domestic;
        Quote quote = carrier.quote(shipment);
        quotes.save(quote);
        return quote;
    }

    private void validate(Shipment shipment) {
        if (shipment.weightKg() <= 0) {
            throw new IllegalArgumentException("weight must be positive");
        }
    }
}

The adapter implementations can now own their carrier-specific authentication, request mapping, and response parsing. The service still owns selection and saving. That is a reasonable split if the carriers truly differ or need separate operational controls. If there is only one carrier and no likely variation, an interface with one implementation may add navigation without reducing change risk. The SOLID principles in practice provide related questions about responsibility and dependency boundaries.

The refactor has not solved every concern. It still needs a rule for duplicate requests, a timeout policy, and a decision about whether failed quotes are persisted. A review should write those open decisions down instead of assuming that smaller classes made the workflow safe.

Responsibility Clarity

Ask each collaborator what business rule or technical boundary it owns. “The quote service coordinates a quote” is more useful than “the service has one responsibility” if the team can then say where validation, carrier selection, conversion, and persistence live.

Look for information passing through a coordinator only to be unpacked elsewhere. If the service reads a shipment’s dimensions, calculates a surcharge, and then writes a result onto the shipment, perhaps a shipping policy or value object should own that calculation. But do not move behavior just to make an object less passive; consider whether the new owner has the needed information and whether the move keeps its purpose coherent. For examples of that assignment decision, see GRASP: Assigning Object Responsibilities.

Coupling, Cohesion, and Change Cost

Coupling is not a raw count of imports. A class depending on a stable domain type can be easier to change than one importing a single vendor SDK whose behavior shifts every quarter. Review the direction and volatility of dependencies: does a business rule know about HTTP status codes, a database session, or a provider’s generated DTO?

Cohesion is also contextual. A QuoteService may reasonably select a carrier and persist a successful quote because those steps form one application workflow. It becomes harder to maintain when unrelated changes, such as rotating credentials, changing a tax rule, and altering database schema, all require editing and retesting its internals.

Trace one change request and record what must change together. If two pieces always change for the same reason, separating them may only add indirection. If one changes independently and repeatedly breaks the other, a seam may pay for itself. This is the same judgment used when reviewing modular monolith boundaries.

Testability Is Evidence, Not the Goal

Ask whether important rules can be tested without starting a web server, database, or carrier sandbox. A test seam is useful when it isolates behavior with a meaningful contract. It is less useful when a mock reproduces private implementation details and makes every harmless refactor break tests.

For the quote flow, test weight validation as a unit, carrier selection as a policy decision, adapter request mapping with contract tests, and persistence behavior at the repository boundary. Add an end-to-end test for the assembled path if the system depends on wiring or configuration. The review should identify what each test proves and what it does not. See metrics, monitoring, and alerting for the operational side of validating production behavior.

Performance and Operational Constraints

An abstraction can improve change isolation and still add runtime cost. Before approving a design that fans out to several providers, loads a large object graph, or caches results, ask what workload it must handle. Use expected volume, latency objectives, payload size, and resource limits to guide a focused measurement. Do not reject a boundary based on imagined overhead or accept it without checking how many network calls the hot path makes.

Review the operational path too. Can an operator tell which carrier failed, distinguish a timeout from a rejected shipment, and correlate a quote with its retry? Are timeout, retry, and fallback policies explicit? Retrying a read-only request may be safe; retrying an operation that creates a booking can duplicate charges or shipments unless it has an idempotency key or provider-supported deduplication. The AWS Well-Architected Framework includes operational excellence, security, reliability, performance efficiency, cost, and sustainability as design concerns; use the dimensions that apply to the workload rather than treating the framework as a scorecard.

Security and Trust Boundaries

Treat external data as untrusted even if it comes from a contracted carrier. Validate response shape and ranges before persisting or returning a quote. Keep credentials in the approved secret store, restrict which component can read them, and make credential rotation possible without changing domain rules. Logs should identify the provider and request correlation ID without copying tokens, customer addresses, or unnecessary personal data.

Review authorization at the operation boundary: which caller may request a quote, for which tenant, and how is that identity carried through the repository query? A clean object model does not provide authorization by itself. For changes that introduce a new data flow or integration, use a structured threat review; the OWASP Threat Modeling Cheat Sheet recommends decomposing the application, identifying and ranking threats, selecting mitigations, and validating the model with stakeholders. The project’s container security guide covers additional deployment controls.

When to Use

• A feature request repeatedly changes several classes, and the team wants to understand whether the changes share a real responsibility.
• A vendor integration, business policy, or storage mechanism changes on a different schedule from the core use case.
• Tests require expensive infrastructure for behavior that should be deterministic and local.
• A production incident exposed unclear ownership of retries, idempotency, authorization, or data validation.
• The team is choosing between a direct implementation and a new abstraction, and needs to compare their maintenance costs.

When NOT to Use

• The feature is small, stable, and clear, and there is no concrete change scenario that a new seam would improve.
• A review is being used to enforce a preferred pattern or class count instead of testing the design against requirements.
• The proposed boundary only renames a call and leaves both sides coupled to the same unstable details.
• A performance concern is hypothetical and no workload or measurement suggests a bottleneck.
• The code is not changing and a redesign would create migration risk without a user or operational benefit.

Production Failure Scenarios

A retry creates duplicate bookings

A carrier call times out after the provider has accepted the booking, so the application retries and creates another booking. Review whether the operation is idempotent, which identifier survives retries, and how operators reconcile an unknown outcome. A generic retry decorator cannot answer those domain questions.

A provider response silently corrupts a quote

An adapter maps a missing currency field to the system default. Quotes appear valid but are interpreted in the wrong currency. A reviewer should ask what validates the provider response, whether unknown values fail closed, and whether the incident can be traced to a response version without logging sensitive payloads.

A new policy bypasses a tenant boundary

Carrier selection moves into a helper that receives only a shipment ID, then fetches a quote without tenant scope. Tests for the new carrier pass, but another tenant’s address can leak into the result. Keep identity and tenant scope explicit at the boundary and test denied access as well as the happy path.

A “clean” design overwhelms the hot path

Each small pricing decision triggers a remote policy lookup. The classes look focused, but quote latency and provider costs rise under load. Review dependency count at runtime as well as source-level coupling. Measure the call path and consider a local policy snapshot or batch operation if the consistency requirements allow it.

Trade-Off Table

Observability Checklist

• Record a correlation ID across the request, adapter call, persistence step, and response.
• Measure quote latency and error rate by provider and outcome class, without using customer identifiers as metric labels.
• Track retries, timeouts, fallback use, and idempotency conflicts separately.
• Log structured decision reasons, such as the selected carrier policy, while excluding secrets and unnecessary personal data.
• Alert on user-visible symptoms and agreed service objectives, not every isolated exception.
• Confirm that support staff can find a quote using an approved identifier and audit the lookup.

Security and Compliance Notes

• Keep authentication and authorization checks close to the use case that exposes the operation.
• Scope repository reads and writes by tenant or principal; do not rely on the caller to filter results afterward.
• Store only the address, shipment, and provider data needed for the quote, with a documented retention period.
• Redact credentials, access tokens, full addresses, and payment data from logs, traces, and exception messages.
• Validate and normalize provider responses before they cross into domain objects.
• Review data residency, audit, and deletion requirements when selecting a provider or cache.
• Threat model new trust boundaries and document mitigations that must be enforced in code or deployment configuration.

Common Pitfalls and Anti-Patterns

Counting patterns as proof

Having a Strategy, Factory, and Repository does not establish that the design is maintainable. Each abstraction needs a reason tied to variation, ownership, or a testable contract. A code smell is a clue to investigate, not an automatic instruction to refactor; Martin Fowler makes that distinction in Code Smell.

Treating SOLID as a class-size formula

One class per verb or one method per interface can create a maze of pass-through objects. Review whether changes that should be independent are isolated, and whether collaborators remain understandable. The SOLID principles in practice article discusses why the principles are heuristics rather than a mechanical score.

Mocking private details

Tests that assert every internal call order can lock the design in place while missing the user-visible rule. Prefer tests of behavior and stable contracts. If a collaborator contract is not meaningful outside the test, reconsider whether the seam belongs in production code.

Optimizing for hypothetical scale

Premature caches, pools, and asynchronous abstractions carry invalidation and debugging costs. State the expected load, measure the bottleneck, and include the new behavior in the operational model before adding complexity.

Quick Recap Checklist

• Name a concrete change or failure scenario the review is meant to address.
• Trace that scenario through callers, domain behavior, integrations, persistence, and telemetry.
• State which object owns each rule and why that owner has the needed information.
• Identify dependencies on volatile details and assess whether they cross a useful boundary.
• Check that responsibilities change together for a reason, not just because they share a file.
• Name the tests that prove the rule, contract, wiring, and operational behavior.
• Review latency, volume, retries, idempotency, and resource costs against actual requirements.
• Check trust boundaries, authorization, data minimization, and retention.
• Record the trade-off and the evidence that would trigger a later redesign.

Interview Questions

Further Reading

• Martin Fowler: Code Smell — why smells prompt investigation rather than automatic refactoring.
• AWS Well-Architected Framework definitions — operational, security, reliability, performance, cost, and sustainability concerns.
• OWASP Threat Modeling Cheat Sheet — a repeatable way to examine design trust boundaries and mitigations.
• Martin Fowler: Refactoring — behavior-preserving steps for improving an existing design.
• Testable Object Design and Collaborator Seams
• GRASP: Assigning Object Responsibilities

Conclusion

Summary

A design review is a conversation about change cost and failure behavior. Follow a realistic scenario through the objects, check responsibility ownership and dependency direction, and make test, performance, operations, and security constraints visible. Add an abstraction when it protects a real boundary; leave a direct implementation alone when it remains clear and stable. Record the trade-off, the evidence behind it, and the condition that would justify revisiting the choice.