Clean Code, Refactoring, and Maintainability Questions

Direct answer. A code smell is a surface signal, not a bug in itself, that usually points to a deeper structural problem: the code works today but will resist the next change. It's a heuristic for WHERE to look, not proof that something is wrong.

Five smells common under deadline pressure

1. Long method -- a function that keeps growing because it's easier to add one more if than to stop and restructure. Remediation: extract by responsibility (see S5) as soon as a function needs a comment to separate its 'sections.'
2. Duplicated code -- the same logic copy-pasted with small tweaks because extracting a shared abstraction felt slower under a deadline. Remediation: extract the shared part once there are two clear copies (the classic 'rule of three' guards against over-extracting on the first duplicate).
3. God object/class -- one class or module that ends up owning unrelated responsibilities because it was the easiest place to bolt on 'just one more thing.' Remediation: split along 'reason to change' (SRP), migrating callers incrementally rather than in one big rewrite.
4. Shotgun surgery -- a single conceptual change (e.g., adding a new payment method) requires touching a dozen files because the concept isn't encapsulated anywhere. Remediation: consolidate the scattered logic behind one seam (a class, interface, or module) so future changes touch one place.
5. Primitive obsession -- passing raw strings/ints around for things that are really domain concepts (an email, a currency amount, a user ID), losing the validation and meaning a real type would carry. Remediation: introduce small value types/wrappers so invalid states become unrepresentable rather than merely 'usually correct.'

Why deadline pressure specifically produces these

Under pressure, the fastest LOCAL change is almost always to keep extending what's already there (one more branch, one more copy-paste, one more method on the class you already have open) rather than to pause and restructure. Each individual shortcut is locally rational; the smell accumulates because nobody's shortcut budget includes 'time to undo the last five shortcuts.'

Trade-offs and pitfalls

• Smells are a starting point for investigation, not an automatic verdict -- a long method that's a single linear sequence of well-named steps with no branching can be more readable than five tiny indirections that force you to jump around a file.
• Don't chase every smell with equal urgency; prioritize by where the churn and bug density actually are (see the complexity-metrics survivor for how to find that objectively) rather than refactoring whatever offends you first.
• Naming a smell is only useful if it's followed by a concrete remediation plan; 'this is a god object' without a proposed split is just a complaint.

Direct answer. Liskov Substitution: anywhere code expects the base type, substituting any subtype must not break that code's correctness -- a subclass must honor the base class's CONTRACT (preconditions, postconditions, invariants), not just its method signatures.

The classic violation: Square extends Rectangle

class Rectangle:
    def __init__(self, w, h): self.w, self.h = w, h
    def set_width(self, w): self.w = w
    def set_height(self, h): self.h = h
    def area(self): return self.w * self.h

class Square(Rectangle):
    def set_width(self, w): self.w = self.h = w   # must keep w == h
    def set_height(self, h): self.w = self.h = h

Any code written against Rectangle reasonably assumes set_width changes ONLY the width:

def resize_width_only(rect: Rectangle):
    original_height = rect.h
    rect.set_width(10)
    assert rect.h == original_height  # true for Rectangle, FALSE for Square

Passed a Square, this assertion breaks -- Square is a valid subtype in the type-system sense (it compiles, it implements every method) but VIOLATES the base class's implicit contract, which is exactly what LSP is about: substitutability of BEHAVIOR, not just of interface.

What breaks for callers

• Any caller who wrote and tested code against Rectangle's documented behavior now has to special-case Square, or worse, ships a latent bug that only manifests when a Square happens to flow through that code path -- often far from where the inheritance decision was made.
• The bug is INSIDIOUS because it passes type checking and basic 'does it implement the interface' review; it only surfaces as a behavioral surprise at runtime.

How to detect and fix

• Detect: look for a subclass that OVERRIDES a method to add a stronger precondition, a weaker postcondition, or a side effect the base class doesn't have (here: 'also changes height'). Tests written against the BASE class's contract, then run against every subclass, catch this automatically.
• Fix: don't force an is-a relationship where the contracts genuinely differ. Square and Rectangle are related conceptually but not substitutable behaviorally; model them as siblings implementing a shared Shape interface (as in the polymorphism example) rather than one inheriting from the other, or make Rectangle immutable (no set_width/set_height at all) so the contract that breaks doesn't exist in the first place.

Trade-offs and pitfalls

• LSP violations are easy to introduce accidentally and hard to spot in code review because the code TYPE-CHECKS; the discipline that catches them is writing tests against the base type's contract and running that same test suite against every subtype ('contract tests').
• Don't over-apply LSP anxiety to prevent all inheritance -- the fix isn't 'never use inheritance,' it's 'only use inheritance where the subtype genuinely honors the supertype's behavioral contract, not just its method signatures.'

Direct answer. Break the query into named, single-purpose stages using CTEs (WITH clauses), give each stage a name that states what it computes, and use descriptive aliases throughout -- so a reader can follow the query top-to-bottom as a sequence of clearly-labeled steps instead of untangling nested joins and aggregations in one dense block.

Before (hard to follow)

SELECT c.region, SUM(o.amount) / COUNT(DISTINCT o.customer_id)
FROM orders o JOIN customers c ON o.customer_id = c.customer_id
WHERE o.status = 'completed' AND o.order_date >= '2026-01-01'
GROUP BY c.region HAVING SUM(o.amount) > 50;

A reader has to mentally track what the un-named SUM(o.amount) / COUNT(DISTINCT o.customer_id) expression MEANS, that the HAVING clause is filtering on a DIFFERENT (unaliased) aggregate than the one being selected, and hold the whole join/filter/aggregate/filter pipeline in their head simultaneously.

After (CTEs, named steps)

WITH completed_orders_this_period AS (
  SELECT customer_id, amount
  FROM orders
  WHERE status = 'completed' AND order_date >= '2026-01-01'
),
revenue_by_customer AS (
  SELECT customer_id, SUM(amount) AS total_revenue
  FROM completed_orders_this_period
  GROUP BY customer_id
),
revenue_by_region AS (
  SELECT c.region,
         SUM(rc.total_revenue) AS region_revenue,
         COUNT(DISTINCT rc.customer_id) AS paying_customers,
         SUM(rc.total_revenue) / COUNT(DISTINCT rc.customer_id) AS avg_revenue_per_customer
  FROM revenue_by_customer rc
  JOIN customers c ON c.customer_id = rc.customer_id
  GROUP BY c.region
)
SELECT region, avg_revenue_per_customer
FROM revenue_by_region
WHERE region_revenue > 50
ORDER BY region;

Verified: both versions return identical results (EU|200.0, US|90.0) against the same test data -- the CTE version is purely a readability refactor, not a behavior change.

Why this is easier to follow

Each CTE name IS documentation: completed_orders_this_period tells you the filter scope without reading the WHERE clause; revenue_by_customer tells you the grain of the next step (per customer, not per order). The final HAVING-equivalent filter is now an explicit WHERE region_revenue > 50 referencing a NAMED column, rather than a repeated, unaliased aggregate expression a reader has to recognize is 'the same thing computed twice.'

Trade-offs and pitfalls

• Breaking a query into many small CTEs can, in some database engines, affect the query planner's optimization choices (materialization behavior varies by engine/version) -- verify performance on realistic data volumes after this kind of refactor, especially on large tables, since readability and performance aren't always aligned.
• Over-fragmenting into CTEs that are each trivially small (a single SELECT *) can add MORE noise than it removes -- aim for each CTE to represent one genuine conceptual STEP in the computation, not an arbitrary line-count-driven split.

Direct answer. Each of these patterns replaces a specific structural smell with an object that owns the varying behavior: Factory replaces scattered new/construction logic with one place that knows how to build the right thing; Strategy replaces a growing conditional with a swappable, self-contained behavior object; Adapter wraps an incompatible interface so callers don't have to special-case it.

Factory: fixing scattered, duplicated construction logic

# Before: construction logic (which concrete model to instantiate) repeated at every call site
if model_type == "linear": model = LinearModel(config)
elif model_type == "tree": model = TreeModel(config)

# After: one place owns the decision
class ModelFactory:
    _registry = {"linear": LinearModel, "tree": TreeModel}
    @classmethod
    def create(cls, model_type, config):
        return cls._registry[model_type](config)

Fixes: duplicated/scattered construction logic (a form of shotgun surgery -- adding a new model type meant editing every call site before; now it's one registry entry).

Strategy: fixing a conditional that keeps growing

class DiscountStrategy:
    def apply(self, total: float) -> float: raise NotImplementedError

class BulkDiscount(DiscountStrategy):
    def apply(self, total): return total * 0.9 if total > 100 else total

class LoyaltyDiscount(DiscountStrategy):
    def apply(self, total): return total * 0.95

def checkout(total, strategy: DiscountStrategy):
    return strategy.apply(total)

Fixes: the exact if/elif-growth problem covered earlier (replace-conditional-with-polymorphism) -- adding a new discount type means a new DiscountStrategy subclass, not editing checkout.

Adapter: fixing an incompatible third-party interface

class LegacyPaymentGateway:
    def do_charge(self, amount_str: str) -> dict: ...

class PaymentGatewayAdapter:
    """Adapts the legacy string/dict interface to the typed interface new code expects."""
    def __init__(self, legacy: LegacyPaymentGateway): self._legacy = legacy
    def charge(self, amount_cents: int) -> ChargeResult:
        raw = self._legacy.do_charge(str(amount_cents))
        return ChargeResult(success=raw["ok"])

Fixes: new code doesn't need to special-case the legacy gateway's stringly-typed, differently-shaped interface at every call site; the adapter absorbs that mismatch in exactly one place.

Trade-offs and pitfalls

• Introducing any of these three for a case with only ONE variant/implementation and no evidence a second is coming is speculative generality -- the pattern earns its keep once there are genuinely multiple interchangeable behaviors or implementations, not preemptively.
• Factory and Strategy can be over-engineered into deep class hierarchies for what could be a simple dictionary of functions in a language with first-class functions -- match the pattern's ceremony to the actual complexity of what's varying.

Direct answer. Pull the calculation into a function that takes its inputs and returns a value with no side effects, and push the side effect (the write) into a thin, separately-named function that does nothing else -- then the pure part is trivially unit-testable with no mocks at all.

Before

STORE = {}
def process(order_id, price, qty, discount_pct):
    total = price * qty
    total -= total * (discount_pct / 100)
    STORE[order_id] = total   # side effect buried inside "calculation"
    return total

Testing this requires a real or fake STORE even though the interesting logic is pure arithmetic.

After

def compute_total(price: float, qty: int, discount_pct: float) -> float:
    """Pure: same inputs always produce the same output, no I/O."""
    total = price * qty
    return total - total * (discount_pct / 100)

def persist_total(store: dict, order_id, total: float) -> None:
    """The only place that touches shared state."""
    store[order_id] = total

def process_after(store, order_id, price, qty, discount_pct) -> float:
    total = compute_total(price, qty, discount_pct)
    persist_total(store, order_id, total)
    return total

Verified: compute_total(100, 2, 10) == 180.0, and the full pipeline produces the identical 180.0 and identical stored value as the original.

What becomes easier to test

• compute_total needs zero setup: assert compute_total(100, 2, 10) == 180.0 is the entire test, no store, no mocking, no cleanup.
• You can now table-test dozens of price/qty/discount combinations cheaply, because none of them touch shared state.
• persist_total becomes small enough that its ONE test just confirms the dict got the right key/value -- you're not re-testing arithmetic every time you test persistence, and vice versa.
• Bugs get localized: if a total is wrong, the bug is in compute_total; if the wrong order_id was written, the bug is in persist_total. In the original, both possibilities are tangled in one function.

Trade-offs and pitfalls

• This split adds one more name and one more call in the composition function -- for a two-line function that's plausibly not worth it, but the moment you have edge cases (negative discounts, currency rounding, tiered discounts) the pure/impure split pays for itself immediately.
• Watch for a subtler version of the same bug: a function that LOOKS pure but secretly reads global mutable state (e.g., a discount rate from a global config) is not actually pure, and hides the same testability problem behind clean-looking code.
• Don't over-purify: a system needs side effects somewhere. The goal isn't zero side effects, it's ISOLATING them so the decision logic can be tested without them.