Code Review and Working with Existing Codebases Questions

Direct answer

Push style and formatting entirely to automated tooling, an auto-formatter and linter run in CI (continuous integration) and ideally on save, so it's never a human opinion sitting in the review thread. Reserve human review time for correctness, design, and readability, and treat a request for a one-off style exception as a signal to either update the rule for everyone or explain why it holds, not as a private negotiation inside one PR.

Structured elaboration

Tooling: an auto-formatter (a tool like Prettier or Black that rewrites code to one canonical style) plus a linter that catches what formatting alone can't, unused variables, inconsistent naming. Both run automatically in CI and block merge on failure; ideally they also run locally on save or commit, so the author never sees a style comment from a human at all.

Process: the team's style rules live in the linter's configuration file, not in people's heads or a wiki page nobody rereads. If reformatting a whole file would drown out a small behavior change in the diff, keep the formatting change in its own separate PR (pull request) so the actual change stays reviewable.

Handling a one-off exception request: treat it as data, not a special case. If it's genuinely justified (a generated file, or a block that's clearer left unformatted), add a scoped ignore with a one-line reason directly in the code or config, so it's documented and repeatable rather than a private favor. If it isn't clearly justified, the answer is: the rule applies to everyone, and if you disagree, propose changing the linter config for the whole team, which keeps the reviewer from personally overriding a shared standard.

Worked example

An author asks to keep a manually aligned table of constants instead of letting the formatter collapse it into the standard one-per-line style. The reviewer checks whether the formatter supports a scoped ignore comment for that block; it does, so it gets added with a one-line reason ("kept aligned for readability, formatter ignored here"). If the formatter had no such escape hatch, the reviewer would decline and suggest raising it as a team discussion about the formatter's config, rather than merging an inconsistent one-off that nobody else knows about.

Trade-offs and pitfalls

Over-configuring the linter with too many exceptions defeats its purpose, and each exception becomes its own small maintenance burden. A common pitfall is letting a senior engineer's exception slide without documenting it, which quietly erodes the "the tool decides, not the person" norm and reopens style debates for everyone else on the team.

Direct answer

This is Python's classic mutable-default-argument bug. def append_to_list(item, lst=[]): looks like a fresh empty list is created every call, but Python evaluates a default argument's value exactly once, when the def statement itself runs, not on every call. Since [] is mutable, every caller who doesn't pass their own lst shares that same one list object across every call to the function, for the lifetime of the program. The fix: default to None, an immutable sentinel, and create a new list inside the function body when the caller didn't provide one.

Structured elaboration

Approach. Python binds a function's default argument values once, at definition time, to the function object itself, not per call. For an immutable default like 0 or None this is invisible, since you can't mutate an int or None in place, so nobody notices the "only once" behavior. For a mutable default like [], {}, or a custom mutable object, it means every call that relies on the default is silently operating on the same shared object, so state written by one caller leaks into every subsequent caller.

Review comment. "Bug: lst=[] is a mutable default argument. Python evaluates it once, at function-definition time, so every call that doesn't pass its own lst shares and mutates the same list, and state leaks between unrelated calls. Please default to None and create a new list inside the function body, and add a regression test asserting two calls with the default argument don't share state."

Worked example

The buggy behavior, demonstrated by actually calling the function twice with no lst argument:

def append_to_list_buggy(item, lst=[]):
    lst.append(item)
    return lst

print(append_to_list_buggy(1))
print(append_to_list_buggy(2))

Output:

[1]
[1, 2]

The second call's result contains 1 from the first call, that's the state leak the review comment flags: the caller of append_to_list_buggy(2) almost certainly expected [2], not [1, 2].

The corrected implementation and a unit test that would have caught the bug:

def append_to_list(item, lst=None):
    if lst is None:
        lst = []
    lst.append(item)
    return lst

def test_append_to_list_does_not_share_state():
    a = append_to_list(1)
    b = append_to_list(2)
    assert a == [1]
    assert b == [2]

test_append_to_list_does_not_share_state()
print("test_append_to_list_does_not_share_state: PASS")

Output:

test_append_to_list_does_not_share_state: PASS

Complexity

Appending one item is O(1) amortized (meaning an occasional slower operation averages out across many calls) time and O(1) additional space per call, aside from the list's own growth. The fix doesn't change that complexity, it changes which list object gets mutated: a fresh one per call instead of one shared object.

Edge cases

• A caller who explicitly passes their own list should still get in-place appending onto that list, that behavior is preserved by both versions.
• A caller who passes something that isn't a list (a tuple, None explicitly) needs a decision: raise a clear error, or duck-type accept anything with an .append method, either is defensible, but it should be intentional and documented, not accidental.
• The bug bites hardest in a loop calling the SAME function repeatedly with no lst argument across many call sites, since the shared list keeps growing without bound, which starts as a correctness bug and becomes a memory-safety concern too.

Trade-offs and pitfalls

The same pattern applies to any mutable default, a dict, a set, or a custom mutable object, not just a list, so a reviewer who's learned to spot this once should generalize the check rather than pattern-matching on lst=[] specifically. A linter (Python's Pylint rule W0102, or Ruff's rule B006) can catch this entire class of bug automatically; it's worth suggesting the team turn that check on in continuous integration (CI) so it doesn't depend on a human catching it in review every time, though that's a process improvement to raise as a follow-up, not a reason to skip flagging this instance now.

Direct answer

As the engineering manager, act on two tracks at once: repair the specific relationship and any public harm quickly and privately, and separately fix the guidelines or process so this failure mode doesn't repeat. Treat a single conversation as the start of the fix, not the end of it, and check back later.

Structured elaboration

1. Talk to the author first, 1:1. Acknowledge the harm directly without minimizing it, and ask what they need right now, whether the comment should be edited or removed, or whether they'd prefer a different reviewer on this PR (pull request).
2. Talk to the reviewer separately, not in a group setting. Be direct that the language was out of line regardless of whether the underlying technical point was right, and get their perspective, rushed, frustrated, unaware of tone, without treating that as an excuse for the impact.
3. Repair publicly if the harm was public. If the comment was visible to the team, a short, genuine acknowledgment of what happened is worth more than a vague "let's all be kind" message that erases the specifics; the goal is for the team to see it was actually addressed.
4. Close the structural gap. Check whether the team's review guidelines say anything about tone and conduct at all. If they don't, that's a process gap, not solely the reviewer's individual failure, and it's the manager's job to close it, e.g. adding an explicit norm plus example phrasing (this is a nit, this is blocking, avoid absolute language like "this is terrible") to the team's review guide.
5. Coach, don't just discipline, for a first occurrence. Work through concrete feedback practices together: phrasing comments about the code rather than the person, leading with a question instead of a command, labeling severity explicitly so a blocking issue doesn't read as optional, and pairing any criticism with a specific alternative rather than a bare complaint. Consider having the reviewer shadow reviews from someone whose feedback style already does this well. If this turns out to be a repeat pattern, it escalates beyond coaching.
6. Follow up and actually measure it. Check in with the author privately again a couple of weeks later, not just once, to see if the relationship genuinely repaired rather than just went quiet. Watch later review threads for whether tone actually changed; a guideline that's written down but never checked tends to fade.

Worked example

A reviewer writes something like "this is embarrassing, did you even test this" on a junior engineer's PR, visible to the whole team channel. The manager messages both people privately within the day, has the reviewer edit the comment, and posts a short, honest acknowledgment in the channel rather than a vague platitude. The team's review guide gets an explicit tone section with example blocking-versus-nit phrasing. Two weeks later, the manager checks in privately with the junior engineer, not just assuming things are fine because nobody's raised it again.

Trade-offs and pitfalls

Over-correcting into a heavily policed review culture, where people are afraid to say anything critical, is its own failure mode; the goal is honest, direct feedback delivered respectfully, not conflict avoidance. Treating this as purely an individual coaching issue without fixing the guideline gap means the next person makes the same mistake. Treating it as purely a documentation fix without a real conversation with both people leaves the actual relationship unrepaired.

Direct answer

A pull request (PR, a proposed code change submitted for review) is too large when a reviewer can't hold it in their head in one sitting. I use three heuristics (rough size, whether it mixes unrelated concerns, and how long a careful read actually takes), and I split along seams the codebase already offers: interface first, behind a flag, refactor separate from behavior change, tests first, and by call-site group.

Structured elaboration

Three heuristics for "too large"

1. Size as a rough proxy: diffs over roughly 300-400 changed lines (excluding generated or vendored files) tend to see review quality drop, not a hard rule, a warning sign
2. Single-concern test: can you describe the PR's purpose in one sentence without the word "and"? "Adds retry logic AND refactors the client AND fixes a typo" is really three PRs
3. Review-time test: would a careful reviewer need more than roughly 30-60 minutes to actually read and reason about every changed line? If mechanically reading it takes that long, understanding the trade-offs and edge cases takes much longer

Five techniques to split a large refactor

1. Interface first: land the new function or class signature as an additive change before the implementation that uses it, so the shape of the change is reviewable on its own
2. Feature flag it: land the new code path disabled by default, so the PR that introduces it carries no behavior change and is low-risk to review and merge
3. Refactor, then behavior: separate pure mechanical changes (rename, extract a method, move a file) with zero behavior change from PRs that actually change behavior, so a reviewer never has to untangle "is this different because of the rename or because of a real change"
4. Tests first: land characterization tests (tests that pin down what the old code currently does, written before touching it) or new tests in their own PR before the implementation change, so the implementation is reviewed against an already-agreed spec
5. By layer or call site: split a change touching many call sites into one PR per logical group (by service or module), migrating incrementally rather than all at once

Worked example

A PR replaces a hand-rolled caching layer with a library-backed one across 12 files and 900 lines. The size heuristic flags it (900 well over 400), and the single-concern test flags it too, it both swaps the cache implementation and renames several variables along the way. Split: PR1 (60 lines) is a pure rename with zero behavior change. PR2 (120 lines) adds the new cache client behind a flag, unused. PR3 (80 lines) adds tests characterizing current cache behavior. PR4-6 (roughly 200 lines each) migrate call sites in three groups by module, flag still off, verified against the characterization tests. PR7 (40 lines) flips the flag on in a low-traffic environment first. PR8 (20 lines) removes the old cache code once the new one has run cleanly for a week. Each PR is independently reviewable in under 30 minutes.

Trade-offs and pitfalls

• Splitting purely by line count, ignoring logical seams, can produce PRs that are individually small but incoherent, harder to review than one clear large PR
• Too many tiny PRs adds coordination overhead (more review requests, more continuous integration (CI, the automated build/test pipeline) runs, more chances to merge out of order), there's a floor below which splitting further isn't worth it
• A "pure refactor" PR only earns that label if it really has zero behavior change; verify that with tests, don't just assert it
• The 30-60 minute review-time heuristic is inherently subjective and varies with reviewer familiarity; use it as a gut check, not a hard gate

Direct answer

Reviewing a memoization pull request (PR) means checking three separate things, not just "does it look right": is the cache key actually correct, is the cache's memory use bounded, and is it safe if the function can be called from more than one thread at once. Leave specific, line-referenced comments with a proposed fix, not a general "looks good."

Structured elaboration

Memoization means caching a function's return value keyed by its input, so a repeat call with the same input is served from the cache instead of recomputed.

Correctness

• Does the cache key capture every input that affects the output, including default/optional arguments and any instance state for a method? A key that's missing one input will return a stale, wrong answer for calls that only differ in that field.
• Is the function actually deterministic and side-effect-free for a given input? If it has a side effect (an increment, a log line, a downstream call), that side effect now only fires on a cache miss, which the PR should call out as an intentional, documented behavior change.
• Is there any invalidation path, or does a cached value live forever even after the underlying data it depends on changes?

Memory safety

• Is the cache bounded, for example an LRU (least-recently-used) cache with a maximum size, or a TTL (time-to-live) that expires old entries, or can it grow without limit and eventually cause an OOM (out-of-memory) failure on a long-running process?
• If the cache keys or values hold references to large or long-lived objects (like an instance whose method is being memoized), does the cache keep those objects alive longer than intended? That's a slow memory leak, not a crash, so it's easy to miss in a quick review.

Thread-safety

• If two threads call the function with the same new argument at almost the same moment, what happens? A plain dict/HashMap being read and written by two threads without synchronization can corrupt its internal structure, not just return a stale value.
• Is the "check cache, then compute, then store" sequence atomic, or can two threads both miss the cache and both do the (possibly expensive) work at once? That's usually acceptable for a pure read-through cache, but it should be a stated decision, not an accident.

Worked example

Concrete review comments for this PR, each pointing at a specific problem and a fix:

• "Nit: please add a test that calls this function twice with identical inputs and asserts the second call returns the cached result without re-running the underlying computation (for example by mocking the expensive call and asserting it was invoked once)."
• "This cache is an unbounded dict, so a long-running process will keep every distinct input in memory forever. Can we bound it, for example with functools.lru_cache(maxsize=...) in Python, or an explicit LRU wrapper in other languages?"
• "This cache isn't synchronized. If this function can be called from more than one thread, either guard the check-then-store sequence with a lock, or use a data structure built for concurrent access (a ConcurrentHashMap in Java, or a dict behind an explicit lock in Python)."
• "Key correctness: the cache key here only includes user_id, but the function's output also depends on as_of_date. Two calls for the same user on different dates will incorrectly return the same cached value."

Trade-offs and pitfalls

A lock around the whole cache is simple but serializes every call, even ones for different keys; a per-key lock or a genuinely concurrent map avoids that contention at the cost of more complex code, worth it once the function is actually called concurrently at meaningful volume, not before. An unbounded cache is the fastest option and the easiest to get wrong in production, since it fails silently (memory just grows) until it doesn't. If the cached value is a mutable object, a caller who mutates what they got back can corrupt the cached copy for every future caller unless the function returns a defensive copy, that's a subtle correctness bug worth its own comment when the return type is mutable.