jcode/skills/code-review/checklist.md

3.3 KiB

Review checklist

Ask these per function. Skip what does not apply; stop at the first real problem in a function and verify it before going on.

Inputs and edge cases

  • Empty ([], "", {}), zero, negative, very large, None/null/undefined?
  • First/last element, off-by-one in ranges and slices, inclusive vs exclusive end?
  • Unicode, whitespace, different line endings, case differences?
  • Duplicate keys/ids, unsorted input, input that changes while iterating?

Errors

  • Which calls can fail (I/O, parsing, network, lookups)? What happens then?
  • Errors swallowed (except: pass, empty catch {}), or turned into a wrong default that flows on (None, 0, "")?
  • Is the error message useful? Is cleanup done on the error path (files/sockets/locks closed - with, try/finally, RAII)?
  • Retries without limit or backoff? Timeouts missing on network/subprocess calls?

State and data flow

  • Is every variable initialised on every path? Used after being freed/closed?
  • Shared mutable state: module globals, class attributes, mutable default arguments, objects passed by reference and then modified?
  • Two representations of the same thing that can drift apart (cache vs source, UI vs model, serialized format vs in-memory)?
  • Save/load and export/import: is every field written AND read back?

Security (wherever input crosses a boundary)

  • Shell: command built from input with string formatting / shell=True?
  • Paths: input joined into a path without checking .. and absolute paths?
  • SQL / HTML / JS: string concatenation instead of parameters / escaping (innerHTML, template strings into the DOM)?
  • Secrets in code, logs or error messages? eval/exec/pickle on input?
  • Downloads/requests: TLS verification disabled, URLs from input (SSRF)?

Concurrency and async

  • Awaited everything that returns a promise/coroutine? Fire-and-forget errors lost?
  • Check-then-act races (exists() then open(), read-modify-write without lock)?
  • Event handlers registered repeatedly (leak) or never removed?

Performance (only if it matters at the expected size)

  • Work inside a loop that belongs outside (re-reading files, re-compiling regexes, O(n^2) membership tests on lists)?
  • Unbounded growth: caches, histories, listeners, logs?

Language specifics

  • Python: is vs ==, late-binding closures in loops, except Exception catching too much, integer vs float division, iterator consumed twice, open() without encoding=.
  • JavaScript: == coercion, this in callbacks, missing await, parseInt without radix, forEach with async callbacks, module scope vs inline onclick, floating point money/coordinates.
  • C/C++: buffer sizes (strcpy, sprintf, missing \0), signed/unsigned comparisons, integer overflow, missing return, ownership/double free, uninitialised memory, sizeof(pointer) vs sizeof(array).
  • Java: == on strings/boxed types, unclosed streams (try-with-resources), equals without hashCode, swallowed InterruptedException.
  • PHP: == juggling, unescaped output (htmlspecialchars), SQL without prepared statements, include with input.
  • Shell: unquoted variables, missing set -euo pipefail, cd without check.

Tests

  • Do tests exist for the changed behaviour? Do they assert the result, or only that nothing crashed? Would they fail if the bug above were present?