Clean Code, Refactoring, and Maintainability Questions

Direct answer. A characterization test (a.k.a. pinning test) doesn't check that code is CORRECT -- it records what the code ACTUALLY does right now, so you have a safety net that catches any accidental behavior change while you refactor code nobody currently understands well enough to describe from memory.

The mechanical workflow

1. Pick a function/method you're about to touch.
2. Write a test that calls it with a realistic input and asserts something you're confident is WRONG (e.g., assertEqual(result, None) when you have no idea what the real result is).
3. Run the test -- it fails, and the failure message shows you the ACTUAL current output.
4. Replace the placeholder assertion with the real observed output.
5. Repeat for a few more representative inputs (including edge cases you can identify by reading the code, even if you don't fully understand WHY it behaves that way).
6. Now you have a test suite that describes current behavior precisely -- not necessarily CORRECT behavior, just CURRENT behavior.

Why this is the right tool for untested legacy code specifically

You can't write a meaningful correctness test for code whose intended behavior nobody can state confidently -- you'd just be guessing at the same time you're trying to change the code. Characterization tests sidestep that: they don't require you to know what SHOULD happen, only to capture what DOES happen, so any refactor that changes that captured behavior gets flagged immediately, giving you room to refactor safely even in total uncertainty about intent.

After the safety net exists

Once characterization tests are in place, you can refactor with confidence that an accidental behavior change will be caught. If, DURING refactoring, you discover the current behavior is actually a bug, that's now a conscious, reviewable decision (update the test AND fix the code, with the change called out explicitly) rather than an accidental regression nobody notices until production.

Trade-offs and pitfalls

• Characterization tests can accidentally 'pin' a bug as if it were intended behavior -- treat any output that looks suspicious as a flag for a follow-up conversation, not silent permanent codification.
• These tests are a TEMPORARY safety net for the refactor, not necessarily permanent regression tests -- some teams keep them, others replace them with intent-based tests once the code's real behavior is understood and confirmed correct.
• Don't skip this step under time pressure with 'I'm confident I understand this function' -- the entire value of the technique is removing that (often false) confidence as a prerequisite for safe change.

Direct answer. Walk the AST looking for the specific structural pattern the anti-pattern represents (an empty or overly-broad exception handler, a function whose branch count exceeds a threshold), report file/line with a clear message explaining WHY it's flagged, and roll it out with an escape hatch so legitimate edge cases aren't blocked outright.

Worked example: detecting empty/bare exception handlers (Python)

import ast

class BareExceptChecker(ast.NodeVisitor):
    def __init__(self):
        self.violations = []

    def visit_ExceptHandler(self, node):
        # bare `except:` has node.type is None; `except Exception: pass` has an empty body
        is_bare = node.type is None
        is_empty_body = len(node.body) == 1 and isinstance(node.body[0], ast.Pass)
        if is_bare or is_empty_body:
            self.violations.append((node.lineno, "bare or empty exception handler"))
        self.generic_visit(node)

def check_file(path):
    tree = ast.parse(open(path).read(), filename=path)
    checker = BareExceptChecker()
    checker.visit(tree)
    return checker.violations

Verified against a small test file containing both except: and except Exception: pass -- both are correctly flagged with their line numbers, while a normal except ValueError as e: log.warning(e) is correctly left unflagged.

Reducing false positives

• Allow an explicit escape hatch (except Exception: # noqa: bare-except -- intentional, see INC-402) so a rare, genuinely-intentional catch-all with a documented reason isn't blocked, but the reason has to be visible right there, not just tribal knowledge.
• Distinguish 'genuinely empty' (pass) from 'logs and re-raises' or 'logs and returns a safe default' -- only the former is the smell; a caught exception that's meaningfully handled shouldn't trip the rule just because it's broad.
• Report the SPECIFIC reason in the message ('bare except swallows ALL exceptions including KeyboardInterrupt and SystemExit'), not just 'violation found,' so a developer understands why to fix it rather than treating it as an arbitrary gate.

Rolling out without blocking every PR

• Run in warn-only mode first, scoped to new/changed lines (see the earlier linting-rollout discussion) so an existing backlog of bare excepts doesn't block unrelated work immediately.
• Provide the escape-hatch comment syntax from day one, so legitimate exceptions to the rule have a sanctioned path rather than developers disabling the whole rule out of frustration.

Trade-offs and pitfalls

• AST-based rules are language/parser-specific and need updating when the language's syntax evolves (new exception-handling syntax, pattern matching) -- budget for maintenance, not a write-once tool.
• A rule that's too aggressive (flagging every broad except Exception even when meaningfully handled) trains developers to route around it with the escape hatch reflexively rather than fixing real issues -- calibrate the rule to the SPECIFIC anti-pattern (empty/swallowing), not broad-catch in general, which is sometimes legitimate.

Direct answer. Linters catch style and correctness-adjacent issues mechanically and instantly (unused variables, obvious bugs, inconsistent formatting); static analysis goes deeper (type errors, security patterns, complexity thresholds) at a real but higher computational cost -- run the fast, cheap checks locally on every save/commit, and the slower, deeper ones in CI where a few extra minutes doesn't block a developer's flow.

The role each plays

• Formatters (Prettier, Black, gofmt): eliminate style debates entirely by making formatting a non-decision -- code is auto-formatted, so nobody reviews or argues about spacing/quote style ever again.
• Linters (ESLint, Pylint, golangci-lint): catch common bugs and style violations fast (unused imports, unreachable code, inconsistent naming) with sub-second feedback, ideal for real-time editor integration and pre-commit hooks.
• Static type checkers (mypy, TypeScript): catch a whole CLASS of bugs (wrong types passed, missing null checks) that a linter typically can't, at the cost of needing type annotations and somewhat longer analysis time.
• Deeper static analysis (security scanners, complexity/duplication analyzers): find subtler issues (SQL injection patterns, high-complexity functions) but can take minutes on a large codebase, so they belong in CI rather than blocking every keystroke.

Why the local-vs-CI split exists

A developer's moment-to-moment feedback loop needs to stay under roughly a second or people disable the tool out of frustration; anything slower belongs in a pre-commit hook (seconds) or CI (minutes), where the cost is amortized against a less time-sensitive point in the workflow. Putting a slow, deep security scan on every keystroke would make the editor unusable; putting NO fast checks locally means every trivial mistake only surfaces after a full CI run, wasting a much longer feedback cycle on something a formatter would have fixed instantly.

Trade-offs and pitfalls

• Running only in CI (nothing locally) means developers discover style/lint issues only after pushing, which is a slower and more frustrating feedback loop than catching them before commit -- invest in local tooling (editor integration, pre-commit hooks) even though it requires more per-developer setup.
• Don't let CI-only checks become 'optional' in practice by making them non-blocking; a security scanner that only warns and never blocks tends to get ignored over time once its warnings become background noise.

Direct answer. Look past 'does it pass tests' to readability, failure handling, API/contract impact, and test coverage of the CHANGE itself -- and phrase feedback as a specific, actionable suggestion rather than a vague complaint.

The core checks, with an example each

1. Readability: would a teammate unfamiliar with this PR understand the intent from the code and names alone? Example feedback: 'This function does three unrelated things (parses, validates, persists) -- can we split it so each has one job? Happy to pair on this if useful.'
2. Test coverage of the CHANGE, not just the file: does the diff itself have tests exercising the new behavior, especially edge cases the author might not have thought of? Example: 'This handles the happy path well -- what happens if items is empty? Could we add a test for that?'
3. API/contract impact: does this change a public function's signature, a response shape, or a database schema in a way that breaks existing callers? Example: 'This response now omits the status field that the mobile client depends on -- can we confirm nothing downstream reads it, or keep it for backward compatibility?'
4. Failure handling: what happens when a dependency is slow, unavailable, or returns something unexpected? Example: 'If this external call times out, does the caller get a clear error, or does this hang indefinitely?'
5. Scope: is this PR doing ONE reviewable thing, or is a refactor bundled with a behavior change, making it hard to tell which lines matter for the actual bug fix? Example: 'Could we split the formatting-only changes into a separate PR so the logic change is easier to review on its own?'

Why 'beyond correctness' matters

A PR that passes its own tests can still be a liability: a correct-today function that's unreadable becomes the next person's bug because they misunderstand it while extending it; a correct-today API change that silently breaks a downstream caller is correct in isolation and wrong in context. Reviewing for maintainability is reviewing for the NEXT change, not just this one.

Trade-offs and pitfalls

• Reviewing everything at maximum scrutiny on every PR doesn't scale -- calibrate depth to risk (a payment-path change deserves more scrutiny than a copy tweak) rather than applying a uniform checklist regardless of stakes.
• Vague feedback ('this could be cleaner') is nearly useless; specific, actionable feedback with a suggested alternative respects the author's time and moves the review forward instead of triggering a guessing game.
• Nitpicking style that a formatter/linter should catch automatically wastes review bandwidth on human judgment that should be spent on the checks above; automate what can be automated so reviewers focus where judgment actually adds value.

Direct answer. Treat this as a change-management program, not a tooling rollout: pair mandatory automation (which removes debate entirely) with voluntary, incentivized skill-building (training, mentoring, visible wins) so the standards feel like a shared craft investment rather than a compliance mandate imposed from outside.

Program design

1. Content: start from a small, high-leverage set of standards (naming, function size, the specific smells causing the MOST real pain in this codebase's incident history) rather than a comprehensive style guide nobody will read -- credibility comes from solving real, felt pain first.
2. Automation for anything mechanical: formatting and simple lint rules are enforced via tooling from day one, removing them entirely from human debate or 'training' -- nobody needs a workshop on tab-vs-space.
3. Training for judgment-based standards: run short, practical workshops (not lectures) using REAL examples from the team's own codebase/incident history, since judgment-based standards (when to extract an abstraction, how to name well) don't automate and need practiced instinct, not just awareness.
4. Mentoring/pairing: pair newer or more skeptical engineers with engineers who already practice these standards well, on REAL work, so the standard is demonstrated in context rather than asserted in a slide deck.
5. Measurable goals: track leading indicators (percentage of PRs passing automated checks without manual intervention, review comment volume on style/naming trending down) and lagging indicators (incident rate, time-to-onboard a new engineer) -- both matter, since leading indicators show adoption and lagging indicators show it's actually working.
6. KPIs with guardrails against gaming: pair any metric with periodic qualitative sampling (an experienced engineer reviews a sample of recent PRs) so metrics can't be satisfied by superficial compliance (renaming one variable to 'pass' a linter check while leaving the underlying design smell untouched).

Building buy-in, not just compliance

• Involve skeptical senior engineers in DEFINING the standards, not just receiving them -- standards that feel imposed by a committee they weren't part of get quiet resistance; standards they helped shape get real ownership.
• Celebrate and publicize concrete wins (an incident that WOULD have happened but didn't, because a new test caught it) so the program has visible, credible evidence of value beyond an abstract mandate.

Trade-offs and pitfalls

• A program that's ALL mandate (enforcement) with no visible value story tends to produce compliance without real behavior change -- people satisfy the letter of the rule while resenting it, which shows up later as pushback the moment enforcement weakens.
• Don't let the KPI dashboard become the goal itself (Goodhart's Law) -- pair quantitative tracking with periodic human judgment sampling so the program stays honest about whether it's actually improving outcomes, not just moving a number.