Code Review and Working with Existing Codebases Questions

Direct answer

When a PR (pull request) touches how user data is stored or retrieved, review for four concrete things, what's collected, who can access it, how it's protected, and how it can be deleted, and insist on evidence for each one in the diff or PR description, since a comment claiming "it's handled" is exactly the gap that causes real incidents.

Structured elaboration

Worked example

A PR adds a "recently viewed items" feature that stores a user's view history in a new table. Review pass: data minimization, does the feature need the full page content, or just an item ID and timestamp? Access control, is the table filtered by the requesting user's ID at the query level, not just hidden in the UI? Logging, a new debug log line dumps the entire view-history payload including the user's session token; flagged and removed. Retention, the existing account-deletion job doesn't touch this new table yet; raised as a blocking comment before merge.

Trade-offs and pitfalls

Treating this checklist as something only a dedicated security or privacy reviewer does means it gets skipped on every "small" PR by everyone else, and most real incidents come from exactly this kind of small, unglamorous change. Blocking every PR pending a full compliance review for genuinely low-risk data is its own failure mode, it trains people to route around the review entirely. Calibrate scrutiny to the actual sensitivity of the specific field, rather than treating all user data as equally high-risk.

A peer-review rubric for extract, transform, load (ETL) or transformation pull requests has to score two different things at once: will this data be reliable, and how much reviewer time will this actually cost. I use seven items on a 0-2 scale (0 = missing, 1 = partial, 2 = complete), split between reliability items that gate the merge and delivery-health items that stay visible but do not block on their own. Whatever can be checked by a machine is enforced in continuous integration (CI); whatever needs judgment goes to a human reviewer.

The seven items

• Tests: 0 = none, 1 = unit tests only, 2 = unit plus an integration run against sample data. CI-enforced; unit tests block the merge, integration tests are required for anything touching a production table.
• Data-quality assertions: 0 = none, 1 = basic null/uniqueness checks, 2 = ranges, referential integrity (a foreign-key-style reference still points to a row that actually exists), and anomaly checks. CI-enforced; a failing assertion must be fixed or explicitly justified with a mitigation plan in the PR description.
• Schema and contract changes: 0 = undocumented or breaking with no migration, 1 = documented with no migration plan, 2 = documented with a migration or backfill plan and backward compatibility. Reviewer-enforced; a breaking change needs a second approver.
• Readability and maintainability: 0 = hard to follow, 1 = readable but undocumented, 2 = clear, modular, with docstrings. Style is linted in CI; architectural clarity is a human judgment call.
• Performance and resource cost: 0 = untested and likely expensive, 1 = a rough estimate, 2 = profiled with cost or latency numbers included. Reviewer-enforced for anything touching a heavy job.
• Observability: 0 = no metrics or logs, 1 = basic logs, 2 = row-count and latency metrics with an updated dashboard or alert. Reviewer-enforced; absence requires a documented follow-up.
• Security and access: 0 = sensitive data handled unsafely, 1 = encryption or access control where needed, 2 = approved masking and least-privilege access reviewed. CI checks for hardcoded secrets; a security owner signs off on anything touching regulated data.

Worked example

Consider a PR that adds a transformation masking a customer email column before it lands in an analytics table, on a dataset of moderate size. Scoring it: tests = 2 (unit tests plus a small integration run), data-quality = 1 (adds a null check but no uniqueness or range assertion), schema = 2 (documented, includes a backfill plan because it changes an existing column), readability = 2, performance = 1 (small change, no profiling numbers attached), observability = 0 (no new metric added for the masking step), security = 2 (masking is exactly what this item exists to reward). Total: 2 + 1 + 2 + 2 + 1 + 0 + 2 = 10 out of a possible 14. Under a threshold like "10 or higher clears with one reviewer," this PR passes with a single approver, but the reviewer should still call out the observability zero explicitly as a fast-follow rather than let a strong total quietly absorb it.

Trade-offs and pitfalls

A pure point-sum threshold can hide exactly that kind of zero on a critical item, so pair the total with a hard floor rule: any single 0 on tests or data-quality forces reviewer sign-off regardless of total. A rubric that is too granular also becomes its own overhead, where reviewers spend more time scoring than reviewing; keep CI-enforceable items automated so a human is only asked to judge genuinely subjective items like readability. Finally, the same rubric should not apply uniformly everywhere: a lighter version fits exploratory or one-off pipelines, and the full rubric belongs on anything that feeds a production or shared table.

Direct answer

A strong answer to "describe a specific code review you participated in recently" picks one real, specific PR (pull request), not a generic description of review habits, and walks through concrete details: what the change was, what role was played, the most significant feedback exchanged, any real disagreement, and how it actually resolved, using technical specifics rather than vague praise like "it went well."

Structured elaboration

Context: name the actual feature or bug in one sentence, specific enough to sound real rather than generic.

Role: was the story from the author's or the reviewer's side; be specific about personal actions taken, not "we."

The key feedback: pick the single most substantive comment exchanged, not a list of minor ones, and explain the reasoning behind it, not just what changed as a result.

Disagreement, if any: say so honestly if there was friction, and describe how it resolved. Interviewers notice when every review is described as friction-free, it reads as either limited experience or a polished but hollow answer.

Outcome: describe what changed in qualitative, specific terms, a bug avoided, a design simplified, a teammate learning something, rather than an invented precise metric.

Worked example

"In a recent review I was the reviewer on a PR that added pagination to an internal reporting API. The author's approach paginated by offset, which is simple but degrades as the table grows and can skip or duplicate rows if data changes between page fetches. I left a comment explaining the specific failure case, a row inserted between two fetches shifts the offset and a row gets silently skipped, and suggested cursor-based pagination keyed on a stable, indexed column instead. The author pushed back that offset was simpler and the table was small today. I agreed it was simpler, but pointed out this endpoint already fed a nightly export job, so a silently skipped row would be a quiet correctness bug, not just a performance nit, later on. We agreed to switch specifically for that reason, and I paired with them for a short session to write the first version of the cursor logic. The result was a more correct API, and because we'd talked through the actual reasoning rather than just handing over a fix, the author used the same cursor pattern on a follow-up endpoint without needing another review comment about it."

Trade-offs and pitfalls

The two most common failure modes: choosing a story with no real substance, a trivial style-only review, which makes an interviewer question the candidate's real review experience; and inventing overly polished outcomes with fabricated precise numbers ("reduced errors by 40%"), which reads as rehearsed rather than genuine. A credible answer usually includes at least a small amount of honest friction along the way.

Direct answer

The core review comment is that a Python-level per-row loop over a table is both slower and more error-prone than a vectorized operation (one that operates on a whole column at once instead of cell by cell), and this specific diff also has a real correctness bug: dividing by a zero-span constant column, plus a dtype issue I reproduced directly. I'd ask for the loop to be replaced with a vectorized min-max scale and a test covering the constant-column edge case.

Structured elaboration

Performance

Iterating with row-by-row indexing inside a Python for loop touches the table one cell at a time through the library's indexing machinery on every iteration, instead of letting the underlying vectorized engine operate on the whole column as a single operation. The fix is a vectorized subtract-and-divide across the columns at once, computed as one operation on the underlying array rather than one Python-level operation per row.

Readability

The loop version buries the actual formula (min-max scaling) inside indexing boilerplate; a reviewer has to mentally simulate the loop to recover what the code is doing. The vectorized version reads as the formula itself, which is what a reviewer, and a future maintainer, actually wants to verify is correct.

Edge cases

• Constant column (max equals min): the formula divides by zero. I verified this concretely: an unguarded vectorized version on a constant column produces a missing-value marker for every entry rather than raising, which is silent and easy to miss. This needs an explicit decision (skip the column, fill with a default, or raise a clear error) rather than an accidental one.
• Integer-typed columns: I actually ran the loop version against integer-typed columns and it raised a type error, because assigning a fractional result back into an integer column cell by cell isn't allowed. This is a real, reproducible bug in the diff as written, not a hypothetical: any caller passing an integer-typed feature column hits it immediately. The vectorized version needs the same fix: cast the columns to a floating-point type before scaling.
• Missing values already present: neither version currently guards against a missing value in the input propagating through the min, max, and arithmetic; worth a comment asking whether that's intended.

Concrete suggestion

import numpy as np

def normalize_features(df, columns):
    df = df.copy()
    df[columns] = df[columns].astype(float)
    col_min = df[columns].min()
    col_max = df[columns].max()
    span = (col_max - col_min).replace(0, np.nan)  # avoid divide-by-zero; NaN signals "cannot scale a constant column"
    df[columns] = (df[columns] - col_min) / span
    return df

Test suggestions

• A normal case with known min/max per column, asserting the scaled values match hand-computed expected values
• A constant column, asserting it comes out as a missing-value marker (or whatever the team agrees on) rather than raising or silently returning garbage
• An integer-typed input column, asserting it doesn't raise, which would have caught the dtype bug above

Worked example

I ran both the original loop version and the suggested vectorized version against the same float-typed sample data to confirm they produce identical results, and separately confirmed the two edge cases above.

import pandas as pd
import numpy as np

def normalize_features_loop(df, columns):
    df = df.copy()
    for col in columns:
        col_min = df[col].min()
        col_max = df[col].max()
        for i in range(len(df)):
            val = df.iloc[i][col]
            df.iloc[i, df.columns.get_loc(col)] = (val - col_min) / (col_max - col_min)
    return df

def normalize_features_vectorized(df, columns):
    df = df.copy()
    col_min = df[columns].min()
    col_max = df[columns].max()
    span = (col_max - col_min).replace(0, np.nan)
    df[columns] = (df[columns] - col_min) / span
    return df

df = pd.DataFrame({
    'feature_a': [10, 20, 30, 40],
    'feature_c': [1, 2, 3, 100],
}).astype(float)

loop_result = normalize_features_loop(df, ['feature_a', 'feature_c'])
vec_result = normalize_features_vectorized(df, ['feature_a', 'feature_c'])
print(np.allclose(loop_result[['feature_a', 'feature_c']], vec_result[['feature_a', 'feature_c']]))
print(vec_result)

Actual output:

True
   feature_a  feature_c
0   0.000000   0.000000
1   0.333333   0.010101
2   0.666667   0.020202
3   1.000000   1.000000

Both versions agree exactly on this data. I also confirmed the dtype pitfall is real: calling the loop version on the original integer-typed columns, without the cast to float first, raises a type error naming the specific fractional value it couldn't place into an integer column, on the very first row where the scaled value isn't a whole number.

Trade-offs and pitfalls

• A vectorized fix that skips the dtype cast just moves the same bug from "raises inside a loop" to "raises, or silently truncates depending on the library version, on the whole-column assignment"; the fix has to address the dtype, not only the loop
• Silently turning a constant column into a missing-value marker is defensible but must be a decision the team agrees on, since a downstream model silently receiving missing features can fail in a much more confusing way, far from this code
• Vectorization sometimes trades a small amount of readability for a large amount of speed on genuinely large data, but here the vectorized version is also more readable, so there's no real trade-off to accept in this specific case

Direct answer

Don't let the disagreement stay abstract: get concrete about what's actually at stake, how many callers exist and how costly a rename would be later, state your reasoning once clearly, and if it's still unresolved, use a fast, low-cost tiebreaker (a quick prototype, a third opinion, or an explicit decision-maker) rather than letting the thread drag on.

Structured elaboration

1. Separate preference from cost. Ask explicitly whether this is reversible. An API used by many callers is expensive to rename later, a breaking change or a long deprecation, so the bar for insisting on a preferred name should scale with how many callers exist and how costly a change would be, not with how strongly either person feels.
2. State your reasoning once, concretely, and ask for theirs. Name the specific downside, not just "I don't like this name": "getUser reads as a lookup that returns null, but this always creates one if missing, that's surprising for the many call sites that assume a pure read."
3. If still unresolved, do a quick, cheap check instead of more debate. Grep the existing callers to see how the API is actually used, or spend ten minutes prototyping both names in real call sites to see which reads better. This replaces opinion with evidence.
4. If still stuck, get a third opinion fast. A tech lead, or whoever owns that part of the API, makes the call; anyone involved can request this, and it's cheap to ask compared to letting the PR (pull request) stall.
5. Once decided, commit and close the thread visibly. Whoever didn't get their preference says so out loud ("okay, going with X") so the disagreement doesn't linger as an unresolved grudge, and the reasoning gets written down briefly (in the PR or a short doc) so the next naming debate doesn't restart from zero.

Worked example

Two engineers disagree on getUser versus findOrCreateUser for a method with roughly forty existing call sites. The reviewer greps the callers and finds most assume a pure lookup with no side effects. Bringing that evidence back, "most callers don't expect this to write anything, the name should signal that," the author agrees the evidence changes the calculus, and they land on findOrCreateUser. A short note is added to the PR explaining the reasoning so a future reader understands why the name is what it is.

Trade-offs and pitfalls

Escalating to a tech lead for every minor naming choice trains the team to stop deciding things themselves. Conversely, letting every naming disagreement burn hours of back-and-forth stalls delivery on something that's often genuinely reversible later behind an alias. The failure mode to avoid is a reviewer pulling rank, "just do it my way, I'm the reviewer," instead of showing the reasoning; that resolves the immediate PR but erodes trust for the next one.