Clean Code, Refactoring, and Maintainability Questions

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. Separation of concerns means each part of the system handles ONE distinct kind of responsibility (parsing input, deciding business rules, storing data) and doesn't reach into another part's job -- so a change to one concern (how you validate a request) doesn't ripple into an unrelated one (how you persist the result).

A concrete before/after

# Before: routing, parsing, business logic, and storage tangled in one handler
def handle_signup(request):
    if request.method != "POST": return error_response(405)
    data = json.loads(request.body)
    if not data.get("email") or "@" not in data["email"]: return error_response(400)
    user_id = str(uuid4())
    db.execute("INSERT INTO users (id, email) VALUES (?, ?)", (user_id, data["email"]))
    return json_response({"id": user_id})

# After: each concern has its own place
def validate_signup(data: dict) -> SignupRequest:      # parsing/validation concern
    if not data.get("email") or "@" not in data["email"]:
        raise ValidationError("invalid email")
    return SignupRequest(email=data["email"])

def create_user(req: SignupRequest, repo: UserRepository) -> str:  # business logic + persistence
    user_id = str(uuid4())
    repo.save(user_id, req.email)
    return user_id

def handle_signup(request, repo):                       # routing concern only
    if request.method != "POST": return error_response(405)
    try:
        req = validate_signup(json.loads(request.body))
        user_id = create_user(req, repo)
        return json_response({"id": user_id})
    except ValidationError as e:
        return error_response(400, str(e))

Each function now answers exactly one question; a change to validation rules touches only validate_signup, a change to storage touches only create_user/UserRepository, and routing/HTTP concerns stay isolated in handle_signup.

How the separation actually helps the next change

Adding a second validation rule (say, a password strength check) means editing validate_signup alone -- no risk of accidentally touching persistence or routing code in the process. Swapping the storage backend means changing UserRepository's implementation without touching validation or routing at all. Each piece can also be UNIT TESTED independently: validate_signup needs no database, create_user needs no HTTP request object.

Trade-offs and pitfalls

• Over-separating a genuinely tiny, stable operation into many tiny functions/files adds navigation overhead (jumping between files to follow one flow) without a corresponding benefit -- the separation should track REAL, independently-changing concerns, not be applied mechanically to every function regardless of size.
• 'Separation of concerns' can be satisfied at multiple altitudes simultaneously (within a function, across classes, across services) -- the same principle that argues for splitting handle_signup into three functions also argues, at a larger scale, for keeping an entire billing SERVICE separate from a notifications service; the reasoning is the same, just the unit of separation changes.

Direct answer. Introduce a seam -- a thin interface or wrapper between your logic and the hard-to-test dependency -- so tests can substitute a fake implementation without touching the real database/API, then test your logic against that seam instead of the real thing.

The problem

def sync_user(user_id):
    conn = ThirdPartyDB.connect()   # real network call, no seam to intercept
    row = conn.query(f"SELECT * FROM users WHERE id={user_id}")
    return transform(row)

There's no place to inject a test double: the function reaches OUT and grabs its own dependency internally, so any test either needs a real (or realistically faked) ThirdPartyDB running, or it can't test transform's logic in isolation at all.

Introducing a seam

class UserRepository:
    def __init__(self, db_client):
        self.db_client = db_client       # injected, not grabbed internally
    def fetch_user_row(self, user_id):
        return self.db_client.query(f"SELECT * FROM users WHERE id={user_id}")

def sync_user(user_id, repo: UserRepository):
    row = repo.fetch_user_row(user_id)
    return transform(row)

A test now passes a UserRepository wrapping a FAKE db_client (an in-memory stub returning a canned row) -- sync_user's actual logic (transform) is tested without any real network dependency, and the seam (UserRepository) can separately get a thin integration test against the real client if desired.

Choosing where the seam goes

Put the seam at the boundary between YOUR logic and the THIRD-PARTY specifics -- not deeper. Here, UserRepository isolates 'how we talk to this specific database'; everything on the sync_user side of that seam is pure application logic, testable with a simple fake that doesn't need to reimplement the third party's actual query semantics.

Trade-offs and pitfalls

• A seam that's too thin (barely wrapping the third-party call) doesn't buy you much; a seam that's too thick (re-implementing significant third-party logic in your fake) risks the fake diverging from real behavior and giving false test confidence -- calibrate the fake to return realistic shapes for the cases your logic actually branches on.
• Introducing a seam is itself a small refactor of legacy code with no tests yet -- do it carefully (ideally under a characterization test on the OUTER function first) so the seam-introduction step itself doesn't silently change behavior.
• Keep at least one lightweight integration test that exercises the REAL third-party client occasionally (even if slow/flaky and run less often), since fakes can drift from reality if the third party's behavior changes and nothing ever re-validates the assumption.

Direct answer. Migrate incrementally by introducing a narrow, explicit interface where the global currently is, routing all access through it, and converting call sites one at a time -- never attempt to delete the global in one step across a codebase that depends on it everywhere.

Why this specific bug is hard to pin down

A global mutable singleton means ANY part of the codebase can read or write shared state at ANY time, so a race condition's trigger might be two completely unrelated modules that happen to touch the singleton concurrently -- the bug's symptom (a corrupted value, an intermittent wrong result) is far from its cause (an unguarded write somewhere else entirely), which is exactly what makes 'intermittent race condition from global state' notoriously hard to debug by inspection alone.

A stepwise migration

1. Wrap the global behind an explicit access interface (a class with getter/setter methods, or accessor functions) even before changing anything about HOW it's stored -- this doesn't fix the race yet, but it gives you a single choke point to instrument and later change.
2. Instrument that choke point (logging, or a debug-build assertion) to find every actual call site touching the shared state, since 'grep for the global variable name' often misses reflection/dynamic access patterns in some languages.
3. Introduce proper synchronization AT THE CHOKE POINT first (a lock, or better, an immutable-value-with-atomic-swap pattern) -- this alone may resolve the race without yet removing the global's global-ness, buying safety quickly.
4. Migrate callers to receive the state via injection (constructor/parameter) instead of reaching for the global directly, one call site or one module at a time, verified independently at each step.
5. Once no caller reaches the global directly anymore, replace it with a properly-scoped instance owned by whichever component actually needs it (often per-request, per-session, or per-worker rather than truly global).

Balancing safety with velocity

Step 3 (synchronize at the existing choke point) often resolves the ACUTE production pain (the race condition) quickly, buying time to do steps 4-5 properly without the pressure of an active incident -- don't let 'we should also remove the global entirely' block shipping the immediate concurrency fix.

Trade-offs and pitfalls

• A lock introduced hastily at the choke point can turn a race condition into a DEADLOCK or a throughput bottleneck if not scoped carefully (locking too broadly, or nesting locks inconsistently across call sites) -- test the fix under realistic concurrent load, not just for absence of the original symptom.
• Don't declare victory once the crash stops reproducing in testing; intermittent concurrency bugs can hide for a long time even after a real fix, and equally, a synchronization fix can mask the symptom without removing the underlying race (e.g., locking the read but not a related write elsewhere) -- confirm the fix addresses the actual mechanism, not just the reproduction you happened to have.

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.