Bad Code, Good Code
Both versions work — only one survives concurrency, scale, time, and failure. Senior judgment lives in the gap.
Bad Code, Good Code
This page is a gallery of worked comparisons: the average implementation and the senior implementation of the same small snippet.
The point is that both versions compile, both pass the happy-path test, and both pass the kind of review that only scans for formatting. The bad version isn't ugly — it looks perfectly normal. The difference only surfaces under:
- Concurrency — when several calls happen at once;
- Scale — when the data grows from 100 rows to 10 million;
- The failure path — when a request times out, throws, or half-succeeds;
- Time — across a DST boundary, a backward clock jump, or a mutation between page loads.
A junior can't see the difference because they read code asking "does it meet the requirement?" A senior reads code asking "what does it do under load, on the unhappy path, at 3am?" In every example below, the difference isn't style — it's the answer to that question.
1. Concurrency & resources
Cache the in-flight promise, not the value
// Average
const cache = new Map();
async function getUser(id) {
if (cache.has(id)) return cache.get(id);
const user = await db.users.find(id); // concurrent calls each hit the DB
cache.set(id, user);
return user;
}// Senior
const inflight = new Map();
function getUser(id) {
let p = inflight.get(id);
if (p) return p; // concurrent calls for one key share a Promise
p = db.users.find(id).catch(err => {
inflight.delete(id); // don't cache a failure forever
throw err;
});
inflight.set(id, p);
return p;
}What the senior sees: there's a gap between if (cache.has) and await. N concurrent callers all see "not cached" and all hit the DB at once — a cache stampede. Storing the in-flight promise coalesces them into a single request (single-flight). And the failure must be evicted, or one transient error gets cached permanently.
Bound the concurrency
// Average
const results = await Promise.all(ids.map(id => fetchDetail(id)));
// with 10,000 ids, that's 10,000 requests fired simultaneously// Senior
async function mapLimit(items, limit, fn) {
const out = new Array(items.length);
let i = 0;
const workers = Array.from({ length: limit }, async () => {
while (i < items.length) {
const idx = i++;
out[idx] = await fn(items[idx]);
}
});
await Promise.all(workers);
return out;
}
const results = await mapLimit(ids, 8, fetchDetail);What the senior sees: Promise.all(map(...)) has unbounded fan-out — it opens as many sockets/connections as there are items. It exhausts the connection pool, trips the other side's rate limit, or OOMs the process. Promise.all isn't a concurrency control; it just means "wait for all of these." The degree of concurrency has to be bounded explicitly.
Put cleanup on the path that runs even when the body throws
// Average
const conn = await pool.acquire();
const rows = await conn.query(sql); // throws → release skipped → connection leaks
pool.release(conn);
return rows;// Senior
const conn = await pool.acquire();
try {
return await conn.query(sql);
} finally {
pool.release(conn); // returned on success and on failure
}What the senior sees: releasing a resource must live in finally (or defer, with, RAII) — the path that runs even when the body throws. The bad version leaks one connection per failed query. The timing is the cruelest part: during an incident the queries are already failing, so the pool drains fast and a small problem escalates into an outage.
Measure durations with a monotonic clock, not the wall clock
// Average
const start = Date.now();
doWork();
const elapsed = Date.now() - start; // can go negative: NTP, DST, a user changing the clock// Senior
const start = performance.now(); // monotonic clock — only ever increases
doWork();
const elapsed = performance.now() - start;What the senior sees: the wall clock can jump backward (NTP correction, DST, a manual change), making the elapsed value negative or nonsensical — and any timeout or metric built on it misbehaves. Use a monotonic clock to measure a duration; use the wall clock only to display a point in time. They are not the same thing.
forEach(async …) does not wait
// Average
items.forEach(async (item) => {
await save(item); // forEach ignores the promise the callback returns
});
console.log('all saved'); // a lie: not one save has finished yet// Senior
await Promise.all(items.map(item => save(item))); // when you want concurrency
// when you need strict order:
for (const item of items) await save(item); // actually awaits one by oneWhat the senior sees: Array.prototype.forEach doesn't know about promises — it does nothing with the promise the callback returns. So the await is decorative: the loop finishes synchronously, the code after it runs before any write has completed, and anything the callback throws is unhandled. Use Promise.all(map) for concurrency, for...of for order.
Check-then-use: the TOCTOU race
// Average
if (fs.existsSync(path)) {
return fs.readFileSync(path); // between the check and the read, the file can vanish
}// Senior
try {
return fs.readFileSync(path); // just do it; handle "missing" via the error
} catch (e) {
if (e.code !== 'ENOENT') throw e;
// handle the missing case
}What the senior sees: there's a time gap between "check that it exists" and "use it" (TOCTOU — time-of-check to time-of-use). In that gap the file can be deleted, replaced, or have its permissions changed — passing the check doesn't mean it still holds at use. The correct shape is to attempt the operation directly and handle failure via the error, not to ask first. The same applies to every check-then-act pair: "check balance then debit," "see the lock then enter the critical section."
Double-checked locking without volatile
// Average (Java) — broken double-checked locking
private static Config instance;
public static Config get() {
if (instance == null) { // first check, unlocked
synchronized (Config.class) {
if (instance == null) {
instance = new Config(); // can be reordered: reference published before construction finishes
}
}
}
return instance;
}// Senior
private static volatile Config instance; // volatile: forbids reordering + guarantees visibility
public static Config get() {
Config local = instance; // read once into a local, saving a volatile read
if (local == null) {
synchronized (Config.class) {
local = instance;
if (local == null) {
instance = local = new Config();
}
}
}
return local;
}What the senior sees: without volatile, instance = new Config() is not one step: allocate memory, write the reference, run the constructor — those can be reordered, so another thread can see instance non-null while the object is still half-constructed. volatile guarantees visibility and forbids that reordering. The lower-effort answer is the initialization-on-demand holder idiom or a language-native lazy singleton, handing the memory model to the runtime.
Multiple resources: close order and double-close
// Average
const file = openFile(path);
const conn = openConn();
doWork(file, conn);
file.close();
conn.close(); // any step above throws → neither closes; and the close order is backwards// Senior
const file = openFile(path);
try {
const conn = openConn();
try {
doWork(file, conn);
} finally {
conn.close(); // last opened, first closed
}
} finally {
file.close(); // first opened, last closed
}What the senior sees: two hazards. First, any step that throws skips the close() calls below it — cleanup has to be in finally. Second, resources should close in reverse order of opening (LIFO), because the later one often depends on the earlier. Where the language has try-with-resources / using / defer / with, prefer it — it guarantees reverse-order, exception-safe close for you. And close() is best made idempotent — closing twice shouldn't blow up.
Every network call needs a timeout
// Average
const res = await fetch(url); // no timeout → if the peer hangs, this hangs forever// Senior
const res = await fetch(url, {
signal: AbortSignal.timeout(5000), // abort after 5s
});What the senior sees: by default a network client waits indefinitely. When the peer is slow or half-dead, the caller's request / connection / thread is pinned — so one slow dependency is amplified into resource exhaustion and a cascading failure on your side. Every cross-process call needs a timeout (plus retries and a circuit breaker). "No timeout" means "timeout = infinity," and that is almost never what you want.
2. Correctness traps
Compute the midpoint without integer overflow
// Average
const mid = Math.floor((lo + hi) / 2); // in fixed-width int languages, lo + hi overflows// Senior
const mid = lo + Math.floor((hi - lo) / 2); // mathematically identical, never overflowsWhat the senior sees: this bug lived in java.util.Arrays.binarySearch for about nine years. In a fixed-width integer language, once the array exceeds ~2³⁰ elements, lo + hi overflows to a negative number. lo + (hi - lo) / 2 is exactly equivalent but never overflows. Even in JS, where integers don't overflow so easily, a senior writes the second form by reflex — because code gets ported.
A comparator must be a total order
// Average
items.sort((a, b) => (a.score > b.score ? 1 : -1)); // returns -1 for equal elements// Senior
items.sort((a, b) => (a.score < b.score ? -1 : a.score > b.score ? 1 : 0));What the senior sees: a comparator that never returns 0 violates the sort contract (antisymmetry, transitivity). The result is an unstable sort, and in an implementation that checks the contract (Java's TimSort) it throws outright — "Comparison method violates its general contract." As an aside, a - b quietly breaks on values that can overflow or produce NaN. Return a genuine three-way result.
Money is not a float
// Average
const total = 0.1 + 0.2; // 0.30000000000000004
const price = 19.99; // binary floating point can't store this exactly// Senior
const totalCents = 10 + 20; // integer arithmetic in the smallest currency unit
const format = cents => (cents / 100).toFixed(2);What the senior sees: never represent money as a binary float — rounding error accumulates, reconciliation comes up a cent short, and nobody can find it. Store integer minor units (or a dedicated decimal type), and make rounding an explicit, documented step at a boundary rather than implicit behavior scattered everywhere.
"Tomorrow" is not "+24 hours"
// Average
const tomorrow = new Date(now.getTime() + 24 * 60 * 60 * 1000); // off by an hour across DST// Senior
const tomorrow = addDays(zonedNow, 1); // let the timezone library handle DST as a calendar dayWhat the senior sees: adding 86,400,000 ms is not the same as adding one calendar day — across a DST boundary a day is 23 or 25 hours. Decide first whether you mean "+24 hours" (an instant) or "the same time tomorrow" (a calendar concept), then pick the matching tool. Conflating the two is one of the most common sources of hard-to-reproduce bugs in scheduling, reminders, and billing.
Compare floats with a tolerance
// Average
if (a === 0.3) { ... } // 0.1 + 0.2 === 0.3 is false// Senior
const EPSILON = 1e-9;
if (Math.abs(a - 0.3) < EPSILON) { ... }What the senior sees: binary floating point can't store most decimal fractions, so after a few operations === almost always misses. Compare with a tolerance (epsilon). One level deeper: the right tolerance depends on magnitude — for large numbers use a relative error, not a fixed absolute one.
The default sort sorts as strings
// Average
[1, 2, 10].sort(); // → [1, 10, 2]// Senior
[1, 2, 10].sort((a, b) => a - b); // → [1, 2, 10]What the senior sees: with no comparator, JS Array.prototype.sort converts elements to strings and compares code units, so 10 lands before 2. A numeric array always needs a comparator. The trap is sneaky because [1, 2, 3] — already sorted, single digits — "looks right," and the bug only shows once two-digit values appear.
Case folding is locale-dependent (the Turkish-I)
// Average (Java)
if (header.toLowerCase().equals("content-type")) { ... }
// in a Turkish locale, "I".toLowerCase() is "ı" (dotless) — the header match fails// Senior
if (header.toLowerCase(Locale.ROOT).equals("content-type")) { ... }What the senior sees: toLowerCase() uses the default locale. For strings with machine meaning — protocol keywords, filenames, identifiers — fold with a locale-independent rule (Java's Locale.ROOT, the equivalent in other languages), or the same code behaves differently on a Turkish machine: the famous "Turkish-I" bug. Save locale-sensitive comparison for human-facing sort.
Integer division truncates
// Average (Java)
double avg = sum / count; // sum, count are int → integer divide first, then widen
int pct = (done / total) * 100; // always 0 when done < total// Senior
double avg = (double) sum / count; // widen before the division
int pct = done * 100 / total; // multiply before dividing to keep precision (watch done*100 overflow)What the senior sees: integer / integer does integer division and drops the remainder — the result is truncated before it's assigned to the double, so the outer (double) is too late. Two common fixes: widen one operand before the division, or multiply before dividing in a pure-integer percentage (while watching the intermediate for overflow). This bug is silent — no error, the answer is just quietly wrong.
Mutating a collection while iterating it
// Average
for (let i = 0; i < list.length; i++) {
if (list[i].expired) list.splice(i, 1); // after the delete, the next element shifts into i and is skipped
}// Senior
const kept = list.filter(item => !item.expired); // produce a new collection; don't touch the one being iterated
// when you must delete in place, walk backward:
for (let i = list.length - 1; i >= 0; i--) {
if (list[i].expired) list.splice(i, 1); // a delete only affects already-visited indices
}What the senior sees: delete an element during a forward pass and the following elements shift left to fill the gap, so incrementing the counter skips one — "two expired items in a row" deletes only the first. In many languages (Java, C#) mutating a collection mid-iteration throws ConcurrentModificationException outright. Either produce a new collection (filter) or iterate backward (a delete doesn't disturb indices you haven't visited yet).
Store instants as UTC, not a naive local string
// Average
saveEvent({ startsAt: '2026-06-05 14:30' }); // a local-time string with no timezone
// who knows which 14:30 this is? a server move or a DST shift makes it wrong// Senior
saveEvent({ startsAt: '2026-06-05T02:30:00Z' }); // an instant → store UTC
// for a recurring event anchored to a place ("9am local every day"), store the zone too:
saveEvent({ startsAt: '2026-06-05T14:30:00', tz: 'Pacific/Auckland' });What the senior sees: storing local time as a string with no zone throws away the critical fact of where — a server timezone change, a DST switch, or a user in another region scrambles it. The rule: an instant is stored as UTC (with Z); a recurring event anchored to a place ("9am local") stores local time plus the IANA zone name (Pacific/Auckland, not +12:00 — the offset moves with DST), converted at display time by a timezone library. Never store a naive local time.
3. Robustness on the failure path
Preserve the cause when re-throwing
// Average
try { await charge(card); }
catch (e) { throw new Error('Payment failed'); } // original stack and reason gone// Senior
try { await charge(card); }
catch (e) {
throw new Error('Payment failed', { cause: e }); // chain the cause
}What the senior sees: re-wrapping without cause throws the stack and the real reason in the bin; whoever is on call ends up with "Payment failed" and nothing to act on. Add the domain context, but always chain the original error on cause.
Make the operation idempotent before you add retries
// Average
await api.charge({ amount, card }); // a retry on timeout double-charges the customer// Senior
await api.charge({ amount, card, idempotencyKey: orderId }); // safe to retryWhat the senior sees: a network timeout is ambiguous — the request may already have succeeded. Blindly retrying a non-idempotent write double-charges. The senior's ordering is: make the operation idempotent first (an idempotency key, or a natural unique constraint), then add retries. The reverse order is an incident.
Retry with backoff, jitter, and only on retryable errors
// Average
for (let i = 0; i < 5; i++) {
try { return await call(); }
catch (e) { await sleep(1000); } // fixed interval; doesn't distinguish error types
}// Senior
for (let attempt = 0; attempt < 5; attempt++) {
try { return await call(); }
catch (e) {
if (!isRetryable(e) || attempt === 4) throw e; // don't retry a 4xx; rethrow on last attempt
const backoff = Math.min(1000 * 2 ** attempt, 30_000);
await sleep(Math.random() * backoff); // full jitter spreads the retry spike
}
}What the senior sees: synchronized, fixed-interval retries create a thundering herd — every client retries on the same beat, hammering an already-struggling service. Exponential backoff with jitter spreads the load. And retrying a non-retryable error (a 400, a validation failure) just wastes attempts and delays the moment the real failure surfaces.
In a batch, one bad item shouldn't sink the rest
// Average
for (const job of jobs) await process(job); // job #3 throws, the other 997 never run// Senior
const results = await Promise.allSettled(jobs.map(process));
const failed = results.filter(r => r.status === 'rejected');
// log the failures, let the rest proceed, return partial successWhat the senior sees: in a batch, one bad item shouldn't stop the other 999. Make the policy explicit — fail-fast or best-effort? When it's best-effort, collect per-item outcomes instead of throwing on the first error.
Don't use exceptions for control flow
// Average
function findUser(id) {
const u = cache.get(id);
if (!u) throw new NotFoundError(); // "not found" is an expected outcome, expressed as an exception
return u;
}
try { return findUser(id); }
catch { return GUEST; } // a try/catch to take the normal branch// Senior
function findUser(id) {
return cache.get(id) ?? null; // "not found" is an ordinary return value
}
const user = findUser(id) ?? GUEST;What the senior sees: exceptions are for the exceptional, not for "an expected alternative outcome." Expressing "not found" or "validation failed" — ordinary branches — as exceptions buries the normal control flow in a catch, forces callers to write if/else as try/catch, and in some runtimes throwing pays for a stack capture. Express expected absence as a value (null, Optional, a Result type); reserve exceptions for the unexpected that genuinely interrupts the flow.
fetch does not throw on an HTTP error status
// Average
const data = await fetch(url).then(r => r.json());
// on 404 / 500, r.json() parses the error page; data is garbage and nobody notices// Senior
const r = await fetch(url);
if (!r.ok) throw new Error(`HTTP ${r.status} for ${url}`);
const data = await r.json();What the senior sees: fetch rejects only on a network-layer failure (offline, DNS); an HTTP 4xx/5xx is, to it, "a response was successfully received" — ok is false but the promise doesn't reject. Calling .json() straight away swallows the error body as data. Always check res.ok / the status before parsing. (Note that libraries like axios throw on non-2xx by default — different behavior, so don't assume across libraries.)
Logging smells
// Average
try { await charge(card); }
catch (e) {
logger.error('charge failed: ' + e); // string-concatenated, loses structure
throw e; // the layer above logs it again → the same error five times
}
logger.info(`user ${email} card ${card.number}`); // card number / PII in the logs// Senior
try { await charge(card); }
catch (e) {
// either handle it here or rethrow — don't both log and rethrow
throw new Error('charge failed', { cause: e });
}
logger.info('charge succeeded', { userId, cardLast4 }); // structured + no sensitive dataWhat the senior sees: three common smells. One, log-and-rethrow: log it low, then rethrow so a higher layer logs it again — the same error appears five times in the logs with mismatched stacks. Either handle it or throw it, not both. Two, writing passwords, tokens, card numbers, or personal data into logs (a compliance incident and a wider leak surface). Three, string concatenation instead of structured fields, so you can't later search by userId or aggregate by status. Logs are for the person debugging at 3am — write to that standard.
4. Data access
Use a cursor for deep pagination, not OFFSET
-- Average
SELECT * FROM orders ORDER BY created_at DESC LIMIT 20 OFFSET 100000;
-- scans and discards 100k rows; inserts/deletes between pages cause duplicates or gaps-- Senior
SELECT * FROM orders
WHERE created_at < :last_seen_created_at
ORDER BY created_at DESC
LIMIT 20;
-- positions by cursor; cost is independent of page depth and stable under concurrent writesWhat the senior sees: OFFSET makes the database read and discard every skipped row — page 5,000 scans 100k rows, and it gets slower the deeper you go. More insidiously, if rows are inserted or deleted between page loads, the OFFSET window shifts and the user sees duplicates or misses rows entirely. Keyset (cursor) pagination is O(page size) and stable under concurrent writes.
The N+1 query
// Average
const orders = await db.query('SELECT * FROM orders WHERE user_id = ?', [userId]);
for (const order of orders) {
order.items = await db.query('SELECT * FROM items WHERE order_id = ?', [order.id]);
}
// 100 orders = 1 + 100 queries// Senior
const orders = await db.query('SELECT * FROM orders WHERE user_id = ?', [userId]);
const ids = orders.map(o => o.id);
const items = await db.query('SELECT * FROM items WHERE order_id IN (?)', [ids]);
const byOrder = groupBy(items, 'order_id');
for (const order of orders) order.items = byOrder[order.id] ?? [];
// always 2 queries, independent of the number of ordersWhat the senior sees: a query per loop iteration is the classic N+1: 100 parent rows become 101 round-trips. The per-round-trip network latency and query-planning cost stack up into the most baffling kind of slow endpoint. Fetch in one batched IN and group by foreign key in memory. In an ORM this is eager loading (include / JOIN FETCH) — and it's off by default, so you have to ask for it.
SQL injection: parameterize the query
// Average
db.query(`SELECT * FROM users WHERE email = '${email}'`);
// email = "x' OR '1'='1" → the whole table walks out// Senior
db.query('SELECT * FROM users WHERE email = ?', [email]); // parameterized: data ≠ codeWhat the senior sees: concatenating user input into a SQL string executes data as code — SQL injection, still at the top of the vulnerability charts. A parameterized query (a prepared statement) separates the data from the statement structure entirely, eliminating the class at the root and letting the database reuse the query plan as a bonus. Hand-rolled escaping isn't a fix; it's a deferral.
Don't do external I/O inside a transaction
// Average
await db.transaction(async (tx) => {
const order = await tx.insert(orders, data);
await emailService.send(order.email); // the transaction stays open, awaiting an HTTP call
await tx.update(...);
});// Senior
const order = await db.transaction(async (tx) => {
const o = await tx.insert(orders, data);
await tx.update(...);
return o; // the transaction wraps only DB work; commit fast
});
await emailService.send(order.email); // external I/O outside the transactionWhat the senior sees: for the life of a transaction the database holds locks and pins a connection. Doing an HTTP call / external service inside it converts "how slow is this external call" directly into "how long are these rows locked"; one hiccup on the peer and lock-waits plus the connection pool blow up. Keep only the must-be-atomic DB work inside; move external I/O outside the boundary. To guarantee "if the DB changed, the email is sent," use the outbox pattern (write the pending event in the same transaction, deliver it afterward) rather than putting the email inside the transaction.
Read-modify-write loses updates
// Average
const acct = await db.get(id);
acct.balance += amount; // two concurrent requests both read the same old value
await db.save(acct); // the later write clobbers the earlier → one credit is lost// Senior
// collapse read-modify-write into one atomic operation
await db.query(
'UPDATE accounts SET balance = balance + ? WHERE id = ?', [amount, id]
);
// or optimistic locking: update with a version column, retry when 0 rows are affectedWhat the senior sees: there's a gap between "read it, modify it in memory, write it back" — two concurrent requests each compute from the same stale value and the later write clobbers the earlier: the classic lost update. Either collapse it into one atomic SET x = x + ? (let the database serialize it), or use optimistic concurrency control (a version column + WHERE version = ?, retry on conflict). Read, then compute in the app, then write back is almost always wrong under concurrency.
5. Scale & performance
Building a string in a loop
// Average (O(n²) in languages with immutable strings)
let csv = '';
for (const row of rows) csv += row.join(',') + '\n'; // each += copies everything so far// Senior
const csv = rows.map(row => row.join(',')).join('\n'); // linearWhat the senior sees: in a language with immutable strings (Java, C#, Python, JS), += in a loop recopies the entire accumulated content each pass — O(n²) total. Invisible at a few rows; at tens of thousands it drops from milliseconds to seconds. Collect the pieces in an array and join once (or use a StringBuilder / write to a stream).
includes in a loop → use a Set
// Average
const result = a.filter(x => b.includes(x)); // each x scans all of b → O(n·m)// Senior
const set = new Set(b);
const result = a.filter(x => set.has(x)); // O(n + m)What the senior sees: repeated includes / indexOf against an array inside a loop is one of the most common sources of an accidental O(n²), and every line looks innocent. Build the repeatedly-queried side into a Set (or Map) once, turning membership from linear into constant.
Recompiling a regex in a hot path / ReDoS
// Average
function isValid(s) {
return new RegExp('^(a+)+$').test(s); // recompiles every call; and this pattern backtracks catastrophically
}// Senior
const VALID = /^a+$/; // hoisted: compiled once; and no nested quantifier
function isValid(s) {
return VALID.test(s);
}What the senior sees: two problems. First, new RegExp(...) in a hot path recompiles the same pattern every call — hoist it out. Second, a nested quantifier like (a+)+ triggers catastrophic backtracking, where a few dozen malicious characters peg the CPU for seconds (ReDoS). Rewrite it into an equivalent form that doesn't backtrack.
Stream a large file, don't read it all into memory
// Average
const data = fs.readFileSync('huge.log', 'utf8'); // the whole file in memory
for (const line of data.split('\n')) process(line);// Senior
const rl = readline.createInterface({
input: fs.createReadStream('huge.log'),
});
for await (const line of rl) process(line); // only one line in memory at a timeWhat the senior sees: reading the whole file at once uses memory proportional to file size — a multi-GB log OOMs the process, and nothing can be processed until the last byte is read (high time-to-first-byte). Streaming line by line flattens memory to a constant and lets you start on line one immediately. The test is simple: when the file's size is set by external input and may be unbounded, don't readFileSync it.
An unbounded cache / unremoved listener is a memory leak
// Average
const cache = new Map();
function memoize(key, compute) {
if (!cache.has(key)) cache.set(key, compute());
return cache.get(key); // only ever grows → unbounded in the number of distinct keys
}// Senior
const cache = new LRUCache({ max: 10_000 }); // bounded, evicts least-recently-used
// for "key is an object whose lifetime drives the entry," use a WeakMapWhat the senior sees: a Map that only ever sets and never evicts is a textbook memory leak — functionally correct, it just grows without bound in the number of distinct keys and OOMs days later. Any long-lived container used as a cache must be bounded (LRU with a cap, or a TTL). Same class of bug: an addEventListener / subscription that's never matched by a removeEventListener / unsubscribe on teardown — the listener keeps the object referenced and it can never be collected.
6. Interface & encapsulation
Don't leak internal mutable state
// Average
class Cart {
#items = [];
getItems() { return this.#items; } // the caller gets the actual internal array
}
cart.getItems().length = 0; // an outsider just cleared your internal state// Senior
class Cart {
#items = [];
getItems() { return [...this.#items]; } // return a copy (or Object.freeze / a read-only view)
}What the senior sees: a getter that hands back the internal collection gives away your encapsulation — any caller can bypass every method you wrote and mutate your state directly, and you'll never know. Return a defensive copy or an immutable view. The cost is one copy; the payoff is that the invariant "state only changes through my methods" actually holds.
Return an empty collection, not null
// Average
function findTags(id) {
if (!has(id)) return null; // every caller must remember the null check
return tags;
}
for (const t of findTags(id)) { ... } // an occasional "not iterable"// Senior
function findTags(id) {
if (!has(id)) return []; // no results is an empty collection
return tags;
}
for (const t of findTags(id)) { ... } // always safeWhat the senior sees: "no elements" is an empty collection, not null. Returning null pushes a null-check onto every caller, and the one that forgets is a null-pointer waiting to happen. Return an empty array/map and callers can iterate unconditionally. Reserve null for meaningful absence ("this field was never set"), not "the collection happens to be empty."
Boolean flag parameters → an options object
// Average
createUser(name, email, true, false, true); // anonymous booleans at the call site// Senior
createUser(name, email, {
sendWelcomeEmail: true,
isAdmin: false,
requireVerification: true,
});What the senior sees: true, false, true at the call site says nothing — a reader has to jump to the signature to learn what each boolean means, and adjacent same-typed parameters are trivially swapped with no compiler complaint. A named options object gives each flag a name at the call site, lets you add options without breaking existing calls, and makes a transposed true/false impossible. A boolean flag parameter is often a signal too: the function does two things and might want to be two functions.
Shallow copy vs deep copy
// Average
const copy = { ...original }; // shallow: nested objects/arrays are still the same reference
copy.tags.push('new'); // original.tags got mutated too// Senior
const copy = structuredClone(original); // deep: nested structures are independent
copy.tags.push('new'); // original is untouchedWhat the senior sees: {...obj} and Object.assign are shallow — they copy the top level only, and nested objects/arrays still share references with the original. So "mutating the copy" silently mutates the original, and these aliasing bugs are miserable to reproduce. When you need a genuinely independent copy, deep-clone (structuredClone, or copy the relevant layers deliberately). Stay clear-eyed the other way too: deep copies aren't free — don't reflexively deep-clone a large structure; ask which layer actually needs to be independent.
Primitive obsession: wrap the concept in a type
// Average
function distance(lat1, lng1, lat2, lng2) { ... }
distance(lng1, lat1, lng2, lat2); // transposed — the compiler is silent, the result is quietly wrong// Senior
function distance(a: Coordinate, b: Coordinate) { ... }
distance({ lat, lng }, { lat: lat2, lng: lng2 }); // a transposition is an error, or it's readableWhat the senior sees: a run of same-typed primitive parameters (four numbers, two strings) is trivially passed in the wrong order, and the type system can't help. Wrap the concept in a type (Coordinate, Money, UserId) and a wrong call is either readable or rejected at compile time — this is the most everyday form of "make illegal states unrepresentable." It also kills a whole class of unit/meaning bugs: meters or miles? cents or dollars? The type nails the answer down.
7. Classic language gotchas
Closing over a loop variable
// Average
for (var i = 0; i < 3; i++) {
setTimeout(() => console.log(i), 0); // prints 3, 3, 3
}// Senior
for (let i = 0; i < 3; i++) {
setTimeout(() => console.log(i), 0); // prints 0, 1, 2
}What the senior sees: var is function-scoped — all three callback closures capture the same i, and by the time they run the loop is long over and i is 3. let gives each iteration a fresh binding, so each closure captures that iteration's value. This is the canonical bug whenever you build functions inside a loop for async callbacks or event listeners.
Mutable default arguments (Python)
# Average
def append(item, target=[]): # the default is evaluated once at def time and shared by all calls
target.append(item)
return target
append(1) # [1]
append(2) # [1, 2] ← not the expected [2]# Senior
def append(item, target=None):
if target is None:
target = []
target.append(item)
return targetWhat the senior sees: Python evaluates a default argument once, at definition time, and every call shares that one object. Use a mutable default (list, dict) and state leaks between calls. The idiom is to default to None and construct the new object inside the body.
parseInt without a radix
// Average
const n = parseInt(userInput); // parseInt('0x10') is 16; historically '08' was octal
const id = parseInt(' 42px'); // → 42 (stops at the first non-digit, no error)// Senior
const n = Number(userInput); // strict: '42px' → NaN, says so when the whole string is invalid
const i = parseInt(userInput, 10); // when you genuinely want "parse a leading integer," pass the radixWhat the senior sees: parseInt has two traps. One: with no radix, a leading 0x is read as hex (and historically a leading 0 as octal), so always pass 10 explicitly. Two: it "parses the leading prefix," so '42px' silently becomes 42 and 'abc' becomes NaN — bad data is swallowed quietly. To ask "is the whole string a valid number," use Number() with a Number.isNaN check.
== does implicit type coercion
// Average
if (userInput == 0) reject(); // '' == 0 is true → empty input is treated as 0
// '0' == false and [] == ![] are also true// Senior
if (Number(userInput) === 0) reject(); // convert explicitly, then ===What the senior sees: == coerces by a complex, counterintuitive set of rules — '' == 0, '0' == false, [] == ![] are all true. Always use === and convert explicitly when you mean to. The one accepted exception is x == null, the idiom that matches both null and undefined.
NaN is not equal to itself
// Average
if (x === NaN) ... // always false
arr.indexOf(NaN) // always -1, even when the array contains NaN// Senior
if (Number.isNaN(x)) ...
arr.includes(NaN) // includes uses SameValueZero and does match NaNWhat the senior sees: NaN is the only value not equal to itself, so any === NaN is false and an indexOf built on === never finds it. Test with Number.isNaN — and not the global isNaN, which coerces its argument first, making isNaN('foo') true and giving false positives.
typeof null === 'object'
// Average
if (typeof x === 'object') x.foo(); // typeof null is also 'object' → crashes when x is null// Senior
if (x !== null && typeof x === 'object') x.foo();
// or: x?.foo()What the senior sees: typeof null === 'object' is a decades-old JS wart. To detect a real object you have to exclude null explicitly. It's also why an x?. optional chain or an x != null guard is often more reliable than typeof.
Java: == compares references, not contents
// Average (Java)
if (a == b) { ... } // compares "same object?", not value// Senior
if (Objects.equals(a, b)) { ... } // compares contents, and null-safeWhat the senior sees: in Java (and C#, etc.) == on objects compares reference identity, not value. String literals share a constant pool, so == is sometimes coincidentally true — the test passes, then a new String("x") in production breaks it. The trap is precisely that it's occasionally right. Compare contents with .equals() (or Objects.equals for null safety).
Java: autoboxing cache (the Integer 127 boundary)
// Average (Java)
Integer a = 127, b = 127;
Integer c = 128, d = 128;
a == b; // true (inside the boxing cache)
c == d; // false (outside the cache — two distinct objects)// Senior
a.equals(b); // compares value, always reliable
// or use the primitive int in comparisons/arithmeticWhat the senior sees: Java caches boxed Integer for -128..127, so == is "coincidentally true" for small values and false past 127. This creates the most insidious kind of bug: unit tests with small numbers all pass, production data goes large and it breaks. Compare boxed values with .equals, and use the primitive int where you can.
String length counts UTF-16 code units
// Average
'👍'.length // 2, not 1
input.slice(0, 10) // may cut an emoji in half → mojibake// Senior
[...'👍'].length // 1 (iterates by code point)
Array.from(str).slice(0, 10) // slice by code point, never splitting a surrogate pair
// for true "user-perceived characters" (combining marks, skin-tone modifiers), use Intl.SegmenterWhat the senior sees: JS .length and index-based slicing count UTF-16 code units, not the characters a human sees. Characters outside the BMP (emoji, some CJK) take two code units, so an index-based cut splits a surrogate pair into garbage — "limit to 140 chars" and "take the first 10 characters" are where this bites. To work in "characters," iterate by code point with the spread operator / Array.from; for rigor (combining marks, ZWJ sequences) segment with Intl.Segmenter.
8. Security
Generate tokens with a CSPRNG, not Math.random()
// Average
const token = Math.random().toString(36).slice(2); // predictable, not for security// Senior
const token = crypto.randomBytes(32).toString('hex'); // cryptographically secure
// browser: crypto.getRandomValues(new Uint8Array(32))What the senior sees: Math.random() is built for speed, not security — its output is statistically random but predictable: a few outputs are enough to infer the internal state and predict the rest. Anything security-relevant (session tokens, password-reset codes, API keys, salts) must use a cryptographically secure source (CSPRNG). The test: if this random value were guessed, is there a security problem? If yes, use a CSPRNG.
Compare secrets in constant time
// Average
if (providedToken === expectedToken) grant(); // returns early on the first differing char → timing side channel// Senior
const a = Buffer.from(providedToken);
const b = Buffer.from(expectedToken);
if (a.length === b.length && crypto.timingSafeEqual(a, b)) grant();What the senior sees: an ordinary string equality (===, memcmp) returns at the first differing byte, so the comparison time leaks how many leading bytes matched. An attacker who times it repeatedly can brute-force the secret byte by byte in polynomial time. Compare secrets — tokens, keys, HMAC signatures — with a constant-time comparison (crypto.timingSafeEqual, Python's hmac.compare_digest) that runs the full length regardless of the result.
How to build this judgment
None of these can be caught by "reading more carefully." What they share is that the bad version works fine in the demo. A senior catches them on sight because they run a few questions over every snippet by default — turn them into a pre-commit checklist:
- Concurrency: what happens if this runs twice at once? Is there a gap between check and act?
- Scale: at 10,000× today's input, how many I/O round-trips and how much memory does this use? Is there a hidden O(n²) or N+1 in here? Is the fan-out bounded?
- Failure path: what happens if this line throws? Does the resource still get released? Is the cause still attached?
- External calls: does every cross-process call have a timeout? Is there external I/O inside a transaction? Is the response status checked?
- Resource bounds: can this cache / collection / listener grow without limit? Is there a cap or cleanup?
- Time: is it still correct across a DST boundary, a backward clock jump, or a mutation between page loads?
- Repeat execution: what happens if this runs twice (a retry, a replay)? Is it idempotent?
- Encapsulation: does this return value or parameter let a caller touch my internal state? Is it a shallow copy or a genuinely independent one? Is "nothing" expressed as an empty collection or as
null? - Environment dependence: does this quietly rely on locale, timezone, the system clock, float precision, or some language-level default?
- Types & equality: is
==used here? can this value beNaNornull? can an equality/type check be tripped by implicit coercion? - Untrusted input: does this value get concatenated into SQL / a command / a path / a regex? Is it parameterized or string-built?
- Cryptography: if this random value were guessed, is there a security problem (→ use a CSPRNG)? Is a secret/signature compared in constant time?
Related: Control Flow, Error Handling, Concurrency, Boundaries.