Code Reviewing

Code Reviewing

Function length, nesting, broad types, hardcoded values, repeated resource construction, or multiple mocks are signals to investigate; they are not defects by themselves.

Contents

• Always Review
• Review When Applicable

Always Review

Requirements and Correctness

• Trace the changed behavior to the user request or user-spec.
• Check happy paths, specified edge cases, failures, and state changes that the change owns.
• Identify behavior that was added accidentally or requested behavior that is missing.
• Separate new regressions from unrelated pre-existing problems.

Scope, Necessity, and Simplicity

• Trace each added behavior, validation, fallback, branch, state, abstraction, dependency, and configuration option to a current requirement, project contract, or realistic condition in the present codebase.
• Report unrequested machinery only when it expands behavior or creates concrete maintenance, correctness, performance, or testing cost. Fewer lines are not automatically simpler; compare responsibilities, states, branches, dependencies, and concepts the solution introduces.
• Check whether the project already has a direct capability that satisfies the requirement. An abstraction used once is acceptable when it expresses a real boundary; it is a problem when it adds indirection or generality without a current use.
• Evaluate the chosen algorithm against realistic input size and project constraints. Report avoidable complexity only when an existing project capability or direct requirement proves it unnecessary and the current choice has a demonstrable consequence; do not prescribe a replacement, speculative optimization, or wholesale redesign.
• Treat handling for extremely unlikely cases as a defect only when no requirement or realistic path justifies it and the extra handling materially complicates the normal path.

Cross-File Contracts

• Read every touched source file in full and the callers, dependencies, schemas, or interfaces on which the change relies. For deleted or renamed files, inspect the supplied change status and diff. For generated, lock, snapshot, or other mechanical artifacts, inspect the supplied diff, generator, and deterministic validation instead of consuming the whole artifact without benefit.
• Verify imports, names, argument order, return values, types, lifecycle assumptions, and error contracts against their definitions.
• Report a mismatch only when it can break behavior, compilation, loading, or a documented contract.

Review When Applicable

Architecture and Maintainability

Apply when the change alters responsibilities, dependencies, public interfaces, or repeated logic.

• Prefer established project architecture over generic pattern preferences.
• Check cohesion, dependency direction, circular dependencies, duplicated responsibility, and abstractions that add indirection without solving a current problem.
• Treat size and nesting as readability signals. Report them only when they hide behavior, make a branch unsafe to change, or prevent useful testing.
• Treat duplicated knowledge or responsibility as a finding only when the copies must change together and a demonstrated divergence risk exists.
• Treat a hardcoded value as a problem when its meaning is unclear, it is repeated as policy, or it should vary by environment; a local obvious value needs no constant ceremony.

Comments and Documentation

• Straightforward code should explain itself through structure and naming.
• A comment is useful when code cannot express why a non-obvious decision exists: a business rule, safety invariant, external constraint, compatibility workaround, deliberate tradeoff, or required ordering.
• The comment should explain the reason and what must remain true. A comment that merely narrates the next statement is noise and should be removed or replaced by clearer code.
• Report a missing comment only when future maintainers could reasonably remove or "simplify" an important constraint because its reason is not recoverable from code or project docs.

Failure Handling and Observability

Apply when the change introduces a failure boundary, external operation, recovery path, or operationally important state transition.

• Errors should be handled where the program can recover, translate them into a stable contract, or add information that is not already available.
• Preserve the original cause when propagating a failure. Do not require a local try/catch that only logs and rethrows; that commonly duplicates logs without improving recovery.
• Check empty catches, lost causes, misleading fallbacks, partial writes, and cleanup on failure.
• Follow the project's logging policy. Require a log when its absence creates a real diagnostic gap, not at every function that calls an API or database.
• Log only the minimum operational context needed. Keep secrets, credentials, sensitive payloads, emails, phone numbers, and unnecessary user identifiers out of logs.