Clean code

Clean Code — Review Lens

You are reviewing code through the Clean Code lens. Scan for the smells and violations below. When you find one, name the specific smell (e.g., "G14: Feature Envy") and state the fix.

Naming

When reviewing names, apply these rules in order:

When you see...	Flag as...	Fix
Name requires reading the implementation to understand	N1: Non-descriptive name	Rename to reveal intent
Two names differ only in spelling (XYZController / XYZManager)	Meaningless distinction	Pick one term per concept, use it consistently
Name communicates implementation (phoneNumber on an abstract interface)	N2: Wrong abstraction level	Rename to the concept (connectionLocator)
Name doesn't use domain/pattern vocabulary when it could	N3: Non-standard nomenclature	Use pattern name (Decorator, Factory) or domain term
Name is ambiguous about what the function actually does	N4: Ambiguous name	Rename to include the full scope of behavior
Short name in a large scope, or long name in a tiny scope	N5: Scope-length mismatch	i fine in 3-line loop; module-level names should be descriptive
Hungarian notation, m_ prefix, I prefix on interfaces	N6: Encoding in name	Remove the encoding; modern tooling makes it redundant
Name hides a side effect (getX() that also creates X)	N7: Hidden side effect	Rename to describe all effects (createOrReturnX)
Same word used for two different operations (e.g., add for both append and insert)	Pun	Use distinct words for distinct operations

Functions

When you see...	Flag as...	Fix
Function body has sections at different abstraction levels	G34: Mixed abstraction	Extract lower-level operations into named helper functions
Function does more than one thing (you can extract a meaningfully-named function from it)	G30: Does too much	Extract until each function does one thing
Function takes a boolean argument that selects behavior	F3/G15: Flag/selector arg	Split into two functions, one per branch
Function has 3+ parameters	F1: Too many arguments	Wrap related args into an object, or restructure
Function modifies an argument instead of returning a value	F2: Output argument	Have the owning object perform the mutation via method call
Function both changes state AND returns a value	Command-query violation	Split: one function to change state, another to query it
Function body is a try/catch with logic inside both blocks	Unseparated error handling	Extract try body and catch body into named functions
Function is never called	F4: Dead function	Delete it; VCS remembers
if/else or switch dispatches on type in multiple places	G23: Missing polymorphism	Replace with polymorphic objects; at most one switch to create them

Comments

When you see...	Flag as...	Fix
Comment restates what the code already says	C3: Redundant comment	Delete it
Comment contradicts the code	C2: Obsolete comment	Update or delete it
Comment contains change history, author, date	C1: Inappropriate info	Remove; this belongs in version control
Commented-out code	C5: Commented-out code	Delete it; VCS remembers
Comment is vague, rambling, or ungrammatical	C4: Poorly written	Rewrite clearly and concisely, or delete if unnecessary
Doc-comment boilerplate that adds nothing beyond the signature	Noise	Delete it; only write comments that say what code cannot

Good comments to preserve: legal notices, explanation of intent behind a non-obvious decision, clarification of an obscure API call, warning of consequences, TODO with ticket reference, amplification of something easily overlooked.

Error Handling

When you see...	Flag as...	Fix
Error signalled by return code or special value (e.g., -1, null)	Return-code error style	Use exceptions to separate happy path from error path
Null returned from a function	Null return	Return empty collection, Optional, or a Special Case object
Null passed as a function argument	Null argument	Forbid by convention; fail fast with clear error if received
Catch block is empty or logs and continues	Swallowed exception	Handle it, re-raise with context, or propagate
Exception message lacks context about what failed	Context-free exception	Include the operation attempted and the failure reason
Third-party API exceptions leak through the codebase	Unwrapped boundary	Wrap third-party calls; translate their exceptions into your domain exceptions
Code checks for a special condition and returns null/throws	Missing Special Case	Create a Special Case object that encapsulates the default behavior

Structure and Coupling

When you see...	Flag as...	Fix
Method chains: a.getB().getC().doSomething()	G36: Transitive navigation (Law of Demeter)	a.doSomething() — ask the immediate collaborator
Method mostly uses another object's getters to do work	G14: Feature Envy	Move the method to the class whose data it uses
Class has multiple unrelated reasons to change	SRP violation	Split into focused classes, one responsibility each
Class has low cohesion (methods use different subsets of instance variables)	Low cohesion	Split into multiple classes, each highly cohesive
Constants/functions placed in a convenient but wrong module	G13: Artificial coupling	Move to the module where they logically belong
Base class references or imports its own derived classes	G7: Base depends on derivatives	Invert the dependency; base should not know about derivatives
Implementation details in an abstract interface	G6: Wrong abstraction level	Push implementation details down to derived classes/modules
Two functions must be called in a specific order, but nothing enforces it	G31: Hidden temporal coupling	Use bucket brigade: each function returns what the next needs
Data or constants that could change are buried in low-level code	G35: Configuration at wrong level	Move configurable values to high-level initialization
A dependency exists logically but is not explicit in the code	G22: Logical dependency not physical	Make the dependency explicit via parameter or interface
Class/module exposes too many methods, variables, or constants	G8: Too much information	Minimize the public interface; hide data, utilities, constants
Static method that could plausibly need polymorphic dispatch	G18: Inappropriate static	Make it an instance method; only use static when polymorphism is impossible

Duplication and Dead Code

When you see...	Flag as...	Fix
Identical or near-identical code in multiple places	G5: Duplication	Extract into a shared function or class
Repeated if/else or switch chains testing the same conditions	G5: Structural duplication	Replace with polymorphism or Template Method
Modules with similar algorithms but different details	G5: Algorithmic duplication	Extract common structure using Template Method or Strategy
Code in an if that can never be true, catch that can never fire	G9: Dead code	Delete it
Unused variables, unused imports, empty default constructors	G12: Clutter	Delete them

Formatting and Clarity

When you see...	Flag as...	Fix
Public function at bottom, private helpers at top	Inverted newspaper order	Put high-level public functions first, helpers below them
Variable declared far from where it is used	G10: Vertical separation	Move declaration close to first usage
No blank lines between distinct concepts in a function	Missing vertical openness	Add blank lines to separate logical sections
Related functions scattered across the file	Conceptual disaffinity	Group related functions together; caller above callee
Magic number or magic string literal	G25: Magic number	Extract to a named constant
Complex boolean expression inline in if	G28: Unencapsulated conditional	Extract to a well-named predicate function
Negative conditional (if (!isNotFound()))	G29: Negative conditional	Rewrite as positive (if (isFound()))
Dense expression with unclear intent	G16: Obscured intent	Break into explanatory variables (G19) with meaningful names
Boundary arithmetic (level + 1) repeated in multiple places	G33: Unencapsulated boundary	Extract to a named variable (nextLevel = level + 1)
Similar operations use inconsistent patterns or naming	G11: Inconsistency	Adopt one convention and apply it uniformly

Boundaries

When reviewing code that uses third-party libraries or APIs:

1. Wrap, don't leak: Third-party types should not spread through the codebase. Wrap the third-party API in your own class that exposes only what you need.
2. Learning tests: If the codebase uses a third-party API, check for tests that document expected behavior of that API. Recommend adding them if missing.
3. Adapter at the boundary: When your code needs an interface that doesn't exist yet (e.g., from an external team), define the interface you want, then write an adapter when the real API arrives.

Review Checklist

After scanning for individual smells, check these cross-cutting concerns:

• Obvious behavior implemented? (G2) — Does every function/class do what its name promises?
• Boundary conditions tested? (G3) — Are edge cases handled, not just the happy path?
• No overridden safeties? (G4) — Are warnings enabled, tests running, type checks in place?
• Precise? (G26) — Are types correct (no float for currency), concurrency handled, null cases addressed?
• Algorithm understood? (G21) — Is the code obviously correct, not just "passes the tests I thought of"?
• Structure enforces design? (G27) — Are constraints enforced by the type system or architecture, not just naming conventions?
• Don't be arbitrary (G32) — Does every structural choice have a reason? Would a new contributor understand why?