Clean Code, Refactoring, and Maintainability Questions

Direct answer

A model-prediction API receiving a request with missing or extra fields should distinguish a genuinely-required field being absent (a hard failure with a specific client error) from an optional field being absent or an unexpected extra field being present (tolerated, with a warning logged), rather than treating every deviation from the exact expected shape as either a total failure or something silently ignored without a trace.

Structured elaboration

Required versus optional, and what happens on each. A feature the model genuinely cannot produce a meaningful prediction without (a required input feature with no sensible default) should reject the request outright with a specific 400 naming the missing field, since attempting to predict on a made-up default for a genuinely-required feature can silently produce a plausible-looking but meaningless prediction, arguably worse than an honest rejection. A feature with a sensible, documented default (an optional field the model can reasonably assume a value for when absent) should be defaulted, with the substitution logged as a warning, not an error, so the request still succeeds.

Warnings versus errors, as a distinct, structured concept. A warning indicates the request succeeded but something about it deviated from the ideal shape (an optional field defaulted, an extra field ignored); this should be surfaced back to the caller in the response itself (a warnings array) so a client integrating against this API can notice and fix its own request shape over time, not just silently succeed forever with a defaulted value it doesn't realize it's getting. An error means the request could not be fulfilled at all and gets a 4xx with no prediction returned.

Client-facing response versus server-side logs. The client-facing error or warning should be specific enough to be actionable ("feature_b is required") without leaking anything about the model's internals (no mention of the model's architecture, feature importances, or internal thresholds). The full detail, including the raw request, is logged server-side with a request ID so an engineer debugging a client integration issue can see exactly what was received, without that raw detail ever appearing in the client-facing response.

Worked example

REQUIRED_FEATURE_KEYS = {"feature_a", "feature_b"}
OPTIONAL_FEATURE_DEFAULTS = {"feature_c": 0.0}

def handle_predict_request(payload: dict) -> dict:
    features = payload.get("features")
    if not isinstance(features, dict):
        raise PredictionRequestError("missing or invalid 'features' object", 400)
    missing = REQUIRED_FEATURE_KEYS - features.keys()
    if missing:
        raise PredictionRequestError(f"missing required feature(s): {sorted(missing)}", 400)
    warnings, normalized = [], {}
    for key in REQUIRED_FEATURE_KEYS:
        value = features[key]
        if not isinstance(value, (int, float)) or isinstance(value, bool):
            raise PredictionRequestError(f"feature '{key}' must be numeric", 400)
        normalized[key] = float(value)
    extra_keys = set(features.keys()) - REQUIRED_FEATURE_KEYS - OPTIONAL_FEATURE_DEFAULTS.keys()
    if extra_keys:
        warnings.append(f"ignored unexpected field(s): {sorted(extra_keys)}")
    for key, default in OPTIONAL_FEATURE_DEFAULTS.items():
        value = features.get(key, default)
        if not isinstance(value, (int, float)) or isinstance(value, bool):
            warnings.append(f"optional feature '{key}' had a non-numeric value, using default {default}")
            value = default
        normalized[key] = float(value)
    prediction = sum(normalized.values())  # placeholder for the real model call
    return {"prediction": prediction, "warnings": warnings, "normalized_features": normalized}

Executed and verified: a request missing feature_b raises a 400 naming exactly that field. A request with feature_a, feature_b, and an unrecognized unexpected field succeeds, returns a prediction, and includes a warning naming unexpected as ignored, rather than silently dropping it with no trace. A request where the optional feature_c is present but non-numeric succeeds by substituting the documented default and logging a warning naming which field was defaulted and why, rather than either failing the whole request over an optional field or silently using a wrong value with no signal to the caller. A request where a REQUIRED field has the wrong type still hard-fails, since a required feature demands a genuine value, not a best-effort guess.

Trade-offs and pitfalls

Defaulting an optional field silently, with no warning surfaced to the caller, is a common shortcut that quietly lets a client's integration bug (consistently omitting a field it should be sending) go unnoticed indefinitely, since the request always appears to succeed; surfacing the warning in the response, even though the request still succeeds, gives the client's own team a chance to notice and fix it. The opposite mistake, rejecting the entire request over any unexpected extra field, is overly brittle for a public or evolving API, where a client sending one additional field it happens to track for its own purposes should not be a hard failure if that field is genuinely irrelevant to the prediction itself.

Direct answer. Decompose by responsibility, not by line count: pull out one function per distinct concern (parse, validate, compute, persist, notify), give each a name that states its single job, and compose them in a thin orchestrator that reads like the original steps.

Before (one function, five jobs)

def handle_order(raw):
    order_id = raw["id"]; email = raw["email"]; items = raw["items"]      # parse
    if not items: raise ValueError("order has no items")                  # validate
    if "@" not in email: raise ValueError("bad email")
    total = sum(i["price"] * i["qty"] for i in items)                     # compute
    if total > 100: total *= 0.9
    DB[order_id] = {"email": email, "total": total}                       # persist
    OUTBOX.append(f"receipt to {email}: ${total:.2f}")                    # notify
    return total

After

def parse_order(raw: dict) -> dict:
    return {"id": raw["id"], "email": raw["email"], "items": raw["items"]}

def validate_order(order: dict) -> None:
    if not order["items"]: raise ValueError("order has no items")
    if "@" not in order["email"]: raise ValueError("bad email")

def compute_total(items, *, bulk_discount_threshold=100, bulk_discount_rate=0.9) -> float:
    total = sum(i["price"] * i["qty"] for i in items)
    return total * bulk_discount_rate if total > bulk_discount_threshold else total

def save_order(db, order_id, email, total) -> None:
    db[order_id] = {"email": email, "total": total}

def send_receipt(outbox, email, total) -> None:
    outbox.append(f"receipt to {email}: ${total:.2f}")

def handle_order_v2(raw, db, outbox) -> float:
    order = parse_order(raw)
    validate_order(order)
    total = compute_total(order["items"])
    save_order(db, order["id"], order["email"], total)
    send_receipt(outbox, order["email"], total)
    return total

Verified against two cases: a two-item order totaling 120 (discounted to 108.0) and a single-item order totaling 20 (no discount) -- both versions return identical totals.

How to confirm you didn't change behavior

• Run the OLD function and the NEW orchestrator against the same set of inputs (including edge cases: empty items, exactly-at-threshold totals, malformed email) and diff the outputs.
• If there's an existing test suite, run it unchanged against the new code first -- a passing suite that never exercised the discount boundary is itself a gap worth flagging, not just a green check mark.
• Where no tests exist yet, write characterization tests against the OLD function first, then refactor, then confirm the same tests still pass -- this way the safety net exists before you touch anything.

Trade-offs and pitfalls

• compute_total's discount threshold is now a named, testable, overridable parameter instead of a buried magic number -- a deliberate improvement, but note it as a design decision so a reviewer knows it wasn't accidental scope creep.
• Over-decomposing (a function per line) makes you trace five files to understand one flow; stop splitting when each piece maps to one clear reason to change.
• The orchestrator itself now has a job: sequencing. Keep it free of business logic so it doesn't quietly become a sixth responsibility.

Direct answer

At the start of a preprocessing pipeline, every numeric and categorical column needs an explicit, recorded policy for its edge cases (NaN, infinity, zero, unseen category) before any modeling logic runs, because silently propagating a NaN or an infinite value downstream turns a data problem into a much harder-to-diagnose modeling problem several steps later.

Structured elaboration

The checklist, per numeric column. Check for NaN (missing value), positive/negative infinity (often from a division by zero or a log of zero upstream), and implausible extreme outliers (a value many orders of magnitude outside the rest of the distribution, which is more often a data-entry or unit error than a real observation). For each, decide explicitly: impute (replace with a reasonable value, such as the column median, when the value is expected to sometimes be missing and downstream code needs a number), clip (cap to a plausible range when extreme values are real but should not be allowed to dominate a model), or drop (remove the row or column entirely when the value cannot be meaningfully repaired and its presence would bias the analysis more than its absence).

The checklist, per categorical column. Check for missing values, unexpected new categories not seen during whatever schema or training process the pipeline assumes, and check that the column's dtype actually matches what the rest of the pipeline expects (a numeric ID column read in as a string is a common silent bug). Decide explicitly: fill missing categoricals with an explicit sentinel like "unknown" (never silently drop the row, unless "unknown" itself would bias the analysis) or reject the row if the categorical value is required for the analysis to be meaningful.

Zeros in denominators specifically. Any ratio or rate computation is a landmine: revenue / sessions where sessions can legitimately be zero produces an infinity or a NaN depending on the numerator, and depending on the library, this can silently propagate through an entire downstream aggregation without ever raising an error. Guard division explicitly (return a defined value like 0 or NaN-with-a-flag for a zero denominator, rather than letting the division itself decide).

Recording the decision for reproducibility. Whatever choice is made (impute/clip/drop) must be logged or stored as pipeline metadata (which columns, which strategy, what threshold), not just applied silently, so that a colleague re-running the same pipeline six months later on new data gets the same treatment and so that an analyst reviewing the results later can see that, for example, three outlier rows were clipped rather than silently vanishing from the final row count.

Worked example

Given impute_missing(df), which fills numeric NaNs with the column median and categorical NaNs with "unknown", the corresponding test suite covers: an all-NaN numeric column (the median of an all-NaN column is itself NaN in pandas, so the function must explicitly decide what to do here, for example fall back to 0 with a logged warning, rather than silently filling NaN with NaN); a DataFrame with mixed dtypes across columns (confirms the numeric-vs-categorical branch is chosen per column, not globally); a DataFrame with no missing values at all (confirms the function is a no-op and does not alter values that did not need imputing); and, critically, a check that the ORIGINAL DataFrame passed in is not mutated in place, since a preprocessing function that mutates its input is a common source of confusing bugs when the same DataFrame object is reused elsewhere in a notebook or pipeline.

import pandas as pd

def test_impute_missing():
    df = pd.DataFrame({"num": [1.0, None, 3.0], "cat": ["a", None, "b"]})
    original = df.copy()
    out = impute_missing(df)
    assert out["num"].iloc[1] == 2.0  # median of [1.0, 3.0]
    assert out["cat"].iloc[1] == "unknown"
    pd.testing.assert_frame_equal(df, original)  # input not mutated

Trade-offs and pitfalls

Clipping outliers can silently hide a real, important signal (a genuine fraud spike looks like an outlier right up until it's the whole point of the analysis), so a clipping threshold should be a documented, deliberate choice, not a default applied uniformly to every numeric column without inspection. The single most common mistake in this checklist is handling NaN but forgetting infinity, since pandas.isna() does not catch np.inf by default, a division-by-zero landmine can produce an infinite value that sails straight through a NaN-only cleaning step and corrupts a downstream mean or standard deviation calculation.

Direct answer. Reduce complexity behind a safety net, not by refactoring blind: pin current behavior with characterization tests first, then extract the distinct decision paths into named, independently-testable pieces (guard clauses, a lookup table, or polymorphism, depending on the shape of the branching), verifying at each step that behavior hasn't changed.

The plan

1. Characterize first: since complexity > 20 usually means many untested or under-tested paths, write characterization tests pinning current output for a representative sample of inputs -- including ones that exercise DEEP branches, not just the common path -- before touching the function's structure.
2. Identify the SHAPE of the complexity: is it a long chain of independent guard conditions (flatten with early returns), a dispatch on a type/category (candidate for a lookup table or polymorphism), or genuinely tangled business logic with real interdependencies (harder -- may need domain input to safely simplify, not just mechanical restructuring)?
3. Extract incrementally, verifying at each step: pull out one clearly-bounded piece at a time (a single validation block, one branch of a decision), re-run the characterization tests, and only proceed once green -- resist the urge to restructure the whole function in one large edit.
4. Re-measure complexity after each extraction to confirm you're actually reducing it, not just moving it into a differently-shaped equally-complex helper.
5. Keep shipping features on the surrounding code by doing this extraction opportunistically alongside real work touching this function, rather than blocking a dedicated 'refactor sprint' that competes with feature delivery.

Confirming behavior is unchanged

• Diff the characterization tests' output before and after each extraction step -- any change is either an intentional, called-out fix (documented as such) or a regression to immediately revert.
• For inputs you can't easily enumerate exhaustively, consider property-based testing (generate many random inputs, assert output equivalence between old and new implementations run side by side) as an additional safety net beyond hand-picked characterization cases.

Trade-offs and pitfalls

• Reducing cyclomatic complexity by mechanically extracting helper functions without addressing WHY the logic is tangled (often: too many responsibilities, or business rules that were bolted on over time without a cohesive model) can just relocate complexity rather than remove it -- watch for 'complexity 22 became five functions each complexity 5' with no actual improvement in how hard the FULL flow is to reason about.
• Don't treat 'reduce complexity' as the end goal in isolation from readability; a function refactored down to complexity 8 that's now split across five oddly-named helpers you have to jump between can be a worse reading experience than a well-organized, slightly-higher-complexity original.

Direct answer. Scale review through clear ownership and automation, not through a single bottleneck reviewer: define who can approve what, automate everything mechanical, and measure the PROCESS itself so it can be tuned rather than assumed to be working.

Design for a 100+ engineer org

1. Ownership model: define CODEOWNERS-style rules so the right domain expert is automatically requested for review, rather than every PR routing to a small pool of 'senior' reviewers who become the bottleneck. Distribute review responsibility as widely as competence allows.
2. Tiered review depth by risk: a copy change or a well-tested internal tool doesn't need the same scrutiny as a payment-path change -- define risk tiers (by directory, by tag, by change type) with correspondingly different required-approval counts and reviewer seniority.
3. Automate everything mechanical: formatting, linting, type checks, security scans, and basic test-coverage deltas run in CI before a human ever looks at the diff, so review time is spent on judgment, not nitpicks.
4. SLAs with escalation, not just aspiration: a target like 'first review response within 1 business day' paired with automated nudges/escalation to a backup reviewer if the primary hasn't responded, so PRs don't silently rot waiting on one person's attention.
5. Bot-assisted triage: auto-labeling by size/risk, auto-assigning reviewers by ownership, and surfacing PRs that have been open too long, so humans manage exceptions rather than manually tracking every PR's status.
6. Feedback loop on the process itself: track review turnaround time, PR size distribution, and post-merge defect rate by review depth -- if a lightweight-review tier correlates with more post-merge bugs, that's a signal to adjust the tiering, not just tighten review everywhere uniformly.

Avoiding the bottleneck

The biggest scaling failure mode is 2-3 'trusted' senior engineers becoming the de facto gate for everything, regardless of the ownership model on paper -- counter this by making the CODEOWNERS routing the actual enforced path (via branch protection), not just a suggestion people route around by pinging their favorite reviewer directly.

Trade-offs and pitfalls

• Too many required approvers on low-risk changes creates exactly the bottleneck this design is trying to avoid; calibrate REQUIRED reviewer count to actual risk, not a blanket 'always get two approvals' policy.
• Metrics like 'time to merge' can be gamed (approving without real engagement to hit an SLA) -- pair turnaround metrics with a periodic qualitative audit (sample a handful of merged PRs and check review comment substance) so the SLA doesn't quietly become a rubber stamp.