Clean Code, Refactoring, and Maintainability Questions

Direct answer. Single Responsibility Principle: a class or function should have exactly one reason to change. 'Responsibility' means an axis of change owned by a specific actor or concern, not literally 'does one thing' in a narrow procedural sense.

What 'a reason to change' actually means

A class that both formats a report AND persists it to disk has two reasons to change: the business wants a different report layout, or the ops team wants a different storage backend. Those changes come from different stakeholders on different timelines. When they're tangled in one class, a storage change risks breaking formatting and vice versa, and two people can't safely work on the two concerns in parallel.

Recognizing a violation on first read

• The class name is vague or conjunctive: UserManager, OrderProcessor, ReportHelperAndSaver -- names with 'and' or generic suffixes like Manager/Handler/Util are a strong tell.
• Methods on the class naturally group into unrelated clusters that never call each other (e.g., half the methods touch a database, half touch an HTTP client, and neither group references the other).
• You can't describe the class in one sentence without using 'and'.
• Changing behavior for one caller's needs forces you to touch code that a completely different caller depends on.

Worked example

A UserService that validates input, applies business rules, persists to the database, AND sends a welcome email has (at least) four responsibilities. Splitting it into a UserValidator, UserRules, a repository, and a WelcomeEmailSender means: a change to email copy touches only WelcomeEmailSender; a change to the persistence layer (say, swapping ORMs) touches only the repository; and each piece can be unit-tested in isolation without mocking the other three.

Trade-offs and pitfalls

• SRP is not 'one method per class' -- that's over-splitting and creates its own maintenance cost (you now have to trace behavior across ten tiny classes instead of one). The right granularity is 'one reason to change,' not 'one line of code.'
• Don't force a split just to satisfy a rule when the two concerns are ALWAYS going to change together in practice; if they share a single reason to change, keeping them together is the correct call, not a violation.
• SRP applies at multiple altitudes: a single function, a class, and a service/module. The 'reason to change' framing works at all three, but the actors change (a function's actor might be 'the caller's contract'; a service's actor might be 'a whole team').

Direct answer. Good version-control hygiene means every commit and PR tells a clear, minimal, reviewable story: small commits with messages that explain WHY, PRs scoped to one reviewable change, and a branching approach the whole team actually follows consistently.

Commit granularity and messages

• Each commit should represent ONE logical change that could, in principle, be reverted independently without breaking something unrelated -- a commit that mixes a bug fix with an unrelated formatting sweep makes both harder to review and harder to revert cleanly later.
• Good message: Fix race condition in order total calculation (see INC-204)\n\nTwo concurrent requests could both read the stale total before either\nwrite committed. Wrap the read-modify-write in a transaction. -- explains WHY (the bug, the mechanism) not just WHAT (which is visible in the diff already).
• Bad message: fix bug -- tells a future reader (including git blame six months from now) nothing they couldn't already see from the diff itself, and gives zero context for WHY this change was needed.

PR size and branch/PR conventions

• Smaller PRs get reviewed faster and more thoroughly -- a reviewer can hold a 100-line diff in their head; a 2,000-line diff gets a rubber-stamp approval because nobody can meaningfully review it in one sitting.
• A consistent branch-naming convention (fix/order-total-race, feature/bulk-export) makes it easy to scan open branches and understand what's in flight without opening each one.
• Rebasing versus merging is a team-level convention choice (rebase keeps history linear and easier to bisect; merge preserves the exact chronological record) -- the specific choice matters less than the TEAM actually agreeing on and following one consistently, so history reads predictably regardless of who wrote it.

Handling large binary or generated files

• Committing large binaries (design assets, model weights, video fixtures) directly into a normal git history bloats every future clone and slows down operations like git log and git blame for the whole team, forever, even after the file is deleted, since git history keeps every version.
• Use Git LFS (or an equivalent large-file extension) for binaries that genuinely need to be version-controlled alongside code, so the main repository only stores a lightweight pointer.
• For anything that can be REGENERATED from source (build output, compiled assets, lockfile-derived artifacts), keep it out of version control entirely via .gitignore rather than committing it and then fighting merge conflicts on a file nobody hand-edits.
• If large files were already committed by mistake, history-rewriting tools (git filter-repo) can remove them retroactively, but that rewrites shared history and needs the same coordination caution as any other history rewrite on a branch others have pulled.

Why this matters for maintainability specifically

Git history is a maintainability tool in its own right: git blame and git log are often the FASTEST way to understand why a confusing piece of code exists, but only if commit messages actually explain the why -- a history of 'fix bug', 'wip', 'more fixes' gives future maintainers nothing to work with when they're trying to understand a decision made months or years ago.

Trade-offs and pitfalls

• Enforcing small PRs can pressure people to under-scope a change that's genuinely indivisible (e.g., a schema migration that must ship atomically with the code that depends on it) -- the goal is REVIEWABLE size, not an arbitrary line-count limit that ignores what a change actually requires.
• Rewriting history (interactive rebase, squashing) before merging to clean up a messy WIP trail is generally good practice, but doing it on a SHARED branch others have already pulled causes real pain -- keep history rewrites scoped to your own not-yet-shared branch.

Direct answer

Defensive programming means writing code that assumes its inputs, callers, and environment will eventually misbehave, and that fails in a controlled, diagnosable way instead of silently corrupting state or crashing somewhere far from the actual mistake. The three patterns interviewers most want to hear are: guard clauses with fail-fast validation, fail-safe defaults, and circuit breakers.

Structured elaboration

Guard clauses and fail-fast validation. Check preconditions at the top of a function and return or throw immediately on invalid input, rather than nesting the happy path three levels deep inside conditionals. This prevents a class of bug where a function silently operates on partially-invalid data because the invalid case was never rejected, it was just never tested. The failure surfaces at the point of the bad input, with a clear message, instead of two call frames later as a confusing null pointer exception.

Fail-safe defaults. When a non-critical piece of configuration or a non-critical dependency is unavailable, degrade to a safe, conservative default rather than propagating the failure. A feature flag service that is down should default to the safest behavior (usually: feature off), not crash the request. This prevents an unrelated dependency's outage from becoming a full outage of your own service.

Circuit breakers. When a downstream dependency starts failing consistently, stop calling it for a cooldown window instead of retrying every request against a dependency that is already down. This prevents cascading failure: without a breaker, a slow or failing downstream call can pile up threads or connections in the caller until the caller itself falls over.

Worked example

Consider a checkout service that calls a fraud-scoring API before completing a purchase. Without defensive programming: the checkout handler passes the request straight to the fraud API, the fraud API starts timing out under load, checkout requests pile up waiting on the timeout, and the whole checkout service runs out of worker threads even though the actual defect is in the fraud API. With the three patterns applied: a guard clause rejects a checkout request with a missing user_id before it ever reaches the fraud API; if the fraud API is unavailable, a fail-safe default routes the order to manual review instead of blocking checkout entirely; and a circuit breaker stops calling the fraud API for 30 seconds once its failure rate crosses a threshold, so checkout degrades to manual review immediately instead of piling up timeouts. The outage that this avoids is a full checkout-service outage caused by a single downstream dependency, which is one of the most common real production incidents.

Trade-offs and pitfalls

Defensive checks are not free. Guard clauses that duplicate the same five checks in ten different functions become their own maintenance burden and are a sign you need a shared validator instead. Fail-safe defaults can hide a real problem if nobody monitors how often the default path is taken (a fraud check that silently defaults to manual review 40% of the time is itself an incident). Circuit breakers add a new failure mode of their own: badly tuned thresholds can trip on a brief blip and reject traffic the dependency could actually have served. The discipline is to add defensive checks at trust boundaries and for dependencies you do not control, not everywhere, and to monitor how often each defensive path actually fires.

Direct answer

A production-ready retry decorator needs to preserve the wrapped function's identity (name, docstring) so it remains debuggable, retry only a bounded number of times with a growing delay between attempts, add randomness (jitter) to avoid many clients retrying in lockstep, and raise the FINAL exception, not the first one, so the caller sees the failure that actually caused the retries to be exhausted.

Structured elaboration

Preserving function metadata. Using functools.wraps (or the equivalent in another language) ensures the decorated function still reports its original __name__ and __doc__; without this, every function wrapped by the decorator would appear as a generic "wrapper" in stack traces, logs, and introspection tools, which makes debugging a production failure meaningfully harder.

Exponential backoff plus jitter. Waiting a fixed delay between every retry means many clients that all failed around the same time (a brief downstream blip) all retry at the same moment, potentially overwhelming the very service that's recovering from an outage; growing the delay exponentially (base_delay, then 2x, then 4x, ...) spaces out retries over time, and adding random jitter (a random value between 0 and the computed delay, rather than the delay exactly) spreads many clients' retries across a window instead of a single instant, which is what actually prevents a synchronized retry storm.

Bounded attempts, and raising the LAST exception. An unbounded retry loop can hang a caller indefinitely against a dependency that is genuinely, permanently down; max_retries bounds the total wait. When retries are exhausted, the decorator must raise the exception from the FINAL attempt, not the first, since the first failure might have been a different, less-informative error (a DNS resolution hiccup) than whatever is actually still failing by the last attempt (a genuine authentication failure, for example), and the caller needs to see the failure that is still current.

Signature and return-value transparency. The decorated function must accept the same arguments and return the same value as the original on success, so that applying the decorator never changes how a function is called or what a successful call returns; the decorator's entire effect should be invisible on the success path and only observable through its retry behavior on the failure path.

Worked example

def retry_with_backoff(max_retries=3, base_delay_seconds=0.1, jitter=True, sleep_fn=time.sleep):
    def decorator(func):
        @functools.wraps(func)
        def wrapper(*args, **kwargs):
            attempt = 0
            last_exc = None
            while attempt <= max_retries:
                try:
                    return func(*args, **kwargs)
                except Exception as exc:
                    last_exc = exc
                    if attempt == max_retries:
                        break
                    delay = base_delay_seconds * (2 ** attempt)
                    if jitter:
                        delay = random.uniform(0, delay)
                    sleep_fn(delay)
                    attempt += 1
            raise last_exc
        return wrapper
    return decorator

Executed and verified (five separate checks): decorator metadata (__name__, __doc__) is preserved on the wrapped function. A function failing twice then succeeding on its third call returns the successful result, having been called exactly three times. A function that always fails is called exactly max_retries + 1 times (one initial attempt plus the retries) and raises the exception FROM THE LAST call specifically (verified by checking the exception message contains "failure #3", the third and final attempt, not "failure #1"). A normal, non-decorated-looking call add(2, 3) still returns 5 on the first try with no retries. With jitter disabled, the exact backoff schedule for a base_delay_seconds=1.0 decorator was confirmed to be [1.0, 2.0, 4.0] seconds between attempts, the expected doubling sequence.

Trade-offs and pitfalls

The most damaging bug in a hand-rolled retry decorator is retrying an operation that is NOT actually safe to retry (a non-idempotent write that could execute twice if the first attempt actually succeeded on the server side but the client never received the response due to a network issue); a retry decorator should generally only wrap operations that are already known to be idempotent, or be paired with an idempotency-key mechanism at the call site, never applied blindly to every network call regardless of whether repeating it is safe. A second common mistake is catching too broad an exception type (bare Exception), which retries errors that will never succeed no matter how many times you try (a malformed-request error, a permission error), wasting time and delaying the failure being surfaced to the caller instead of failing fast on those.

Direct answer. Comment when the code can't express WHY (a business rule, a workaround, a non-obvious trade-off); refactor instead of commenting when the comment only restates WHAT the code already says -- a comment that duplicates the code is guaranteed to drift out of sync with it eventually.

The trivial case

# increment i by 1
i += 1

This comment is pure noise: it tells you nothing i += 1 doesn't already say faster to read. Delete it; if i needs a better name to convey intent (e.g., retry_count += 1), fix the name instead of commenting around it.

A comment that legitimately belongs (explains WHY)

# Stripe requires idempotency keys to be reused for retries within 24h,
# otherwise it treats a retry as a new charge. See INC-4021.
idempotency_key = order_id  # intentionally NOT time-based

No amount of renaming makes 'why we chose this specific value, tied to an external API's undocumented-until-we-got-burned behavior' obvious from the code alone -- this is exactly the kind of context a comment should preserve, ideally with a link to the incident/ticket for anyone who wants the full story.

A comment that's a smell (explains WHAT, redundant with the code)

# loop through all users
for user in users:

The code already says this as clearly as English could; the comment adds a second thing that has to be kept in sync every time the loop changes, for zero reader benefit.

A simple test to apply

Ask: 'if I deleted this comment, would a competent reader lose information, or just lose a restatement?' If deleting it loses nothing, delete it. If deleting it loses the REASON something non-obvious is true, keep it (and consider whether the reason belongs in a commit message / ticket link too, for permanence).

Trade-offs and pitfalls

• Comments that explain why are still at risk of going stale if the underlying reason changes (the external API behavior gets fixed) but nobody removes the now-obsolete comment -- treat comments as code that also needs maintenance, not a write-once artifact.
• Don't over-correct into a 'no comments ever' culture; some domains (financial regulations, security-sensitive code, deliberately non-obvious performance tricks) genuinely need WHY documented, and a codebase that bans comments entirely just pushes that knowledge into people's heads (or nowhere), which is worse.
• A comment that says 'TODO: fix this properly' with no ticket link or date is close to noise too -- if it's worth flagging, it's worth tracking somewhere more durable than an inline string that nobody searches for.