Review Module Design
Overview
Core Principle: Evaluate code by detecting complexity symptoms and structural anti-patterns. Good design minimizes dependencies, eliminates obscurity, and maximizes module depth.
Evaluation Focus: What's wrong with this code? Why is it hard to understand or modify?
When to Use
- Reviewing code (PR review, code review)
- Assessing interface quality
- Answering "is this too complex?"
- Evaluating design decisions
- User says: "review", "check my design", "is this good?", "PR review"
When NOT to Use
- Creating new modules (see: designing-deep-modules)
- Transforming/refactoring code (see: simplifying-complexity)
- Adding documentation (see: improving-code-clarity)
- Performance optimization (see: optimizing-critical-paths)
Evaluation Checklist
Use this systematic checklist when reviewing code:
1. Complexity Symptoms (Ch2)
| Symptom | Question | If Yes |
|---|---|---|
| Change Amplification | Does a simple change require modifications in many places? | Flag dependency problem |
| Cognitive Load | Must developer know too much to work here? | Flag obscurity or leaky abstraction |
| Unknown Unknowns | Is it unclear what code/info is needed for changes? | Highest severity - flag immediately |
2. Module Depth (Ch4)
| Check | Deep (Good) | Shallow (Bad) |
|---|---|---|
| Interface vs implementation | Interface much simpler | Interface rivals implementation |
| Method count | Few, powerful methods | Many, limited methods |
| Hidden information | High | Low |
| Common case | Simple to use | Complex to use |
Red flag: If understanding the interface isn't much simpler than understanding the implementation, the module is shallow.
3. Information Hiding (Ch5)
| Red Flag | Detection | Severity |
|---|---|---|
| Information Leakage | Same knowledge in multiple modules | High |
| Temporal Decomposition | Structure mirrors execution order rather than knowledge | Medium |
| Back-Door Leakage | Shared knowledge not visible in interfaces but both depend on it | High |
| Overexposure | Common use forces learning rare features | Medium |
4. Layer Abstraction (Ch7)
| Red Flag | Detection | Severity |
|---|---|---|
| Pass-Through Method | Method only passes arguments to another with same API | High |
| Adjacent Similar Abstractions | Following operation through layers, abstractions don't change | High |
| Shallow Decorator | Large boilerplate, small functionality gain | Medium |
Test: Follow a single operation through layers. Does the abstraction change with each method call? If not, there's a layer problem.
5. Together/Apart (Ch9)
| Red Flag | Detection | Severity |
|---|---|---|
| Conjoined Methods | Can't understand one method without another's implementation | High |
| Special-General Mixture | General mechanism contains use-case specific code | High |
| Code Repetition | Same code appears in multiple places | Medium |
| Shallow Split | Method split resulted in interface ≈ implementation | Medium |
Red Flags Quick Reference
| Red Flag | Source | One-Line Detection |
|---|---|---|
| Shallow Module | Ch4 | Interface as complex as implementation |
| Classitis | Ch4 | Many small classes, little functionality each |
| Information Leakage | Ch5 | Same knowledge in multiple modules |
| Temporal Decomposition | Ch5 | Structure follows execution order |
| Pass-Through Method | Ch7 | Method just delegates to another with same API |
| Conjoined Methods | Ch9 | Methods only understandable together |
| Special-General Mixture | Ch9 | General mechanism has use-case code |
| Code Repetition | Ch9 | Same code appears multiple places |
| Shallow Split | Ch9 | Method split resulted in interface ≈ implementation |
Together/Apart Decision Procedure
When evaluating whether code should be combined or separated:
1. Do pieces share information?
YES → Should probably be together
2. Would combining simplify the interface?
YES → Should probably be together
3. Is there repeated code?
YES → Extract shared method (if long snippet, simple signature)
4. Does module mix general-purpose with special-purpose?
YES → Should be separated
Key principle: Depth > Length. Never sacrifice depth for length.
Depth vs Length Rule
| Situation | Correct Action |
|---|---|
| Long method with clean abstraction | Keep together |
| Short method requiring another's impl to understand | Combine them |
| Method split creating conjoined pair | Undo the split |
| Long method with extractable subtask | Extract subtask only |
Test for valid split: Can the pieces be understood independently AND reused separately?
Evaluation Output Format
When reporting findings, use:
## Design Review: [Component Name]
### Critical Issues (Must Address)
- [Red flag]: [Specific location] - [Why it's a problem]
### Moderate Issues (Should Address)
- [Red flag]: [Specific location] - [Why it's a problem]
### Observations (Consider)
- [Pattern noticed] - [Potential concern]
### Positive Patterns
- [What's working well]
Before Flagging a Problem
Before reporting any red flag, validate:
- Steel-man check: What's the best argument this design choice is intentional?
- Intentional shallowness: Is this an adapter, facade, or decorator where thinness is the point?
- Testing seam: Is this "leakage" actually a legitimate dependency injection point?
- Abstraction quality: Can callers use this interface correctly without knowing implementation details?
Cross-Module Analysis
Ask before concluding:
- Are there related modules that should be reviewed together? Classitis often hides across file boundaries.
- Would combining these modules simplify the overall interface? If yes, flag as potential shallow split.
- Must callers use these modules in sequence? If yes, possible temporal decomposition.
When Principles Conflict
| Conflict | Resolution |
|---|---|
| Depth vs Cohesion | Prefer cohesion. A focused shallow module beats a bloated deep one. |
| Information Hiding vs Testability | Testing seams (injectable dependencies) are acceptable "leakage" |
| Simple Interface vs Configurability | Real systems need configuration; penalize only unnecessary complexity |
Common Evaluation Mistakes
| Mistake | Reality |
|---|---|
| "It's long, so it's complex" | Length ≠ complexity. Depth matters more. |
| "Small classes are better" | Small classes are often shallow. Deep > small. |
| "More methods = better API" | Fewer powerful methods beat many limited ones. |
| "Separation is always good" | Subdivision has complexity costs. Sometimes combine. |
| "Code reuse requires extraction" | Only extract if snippet is long AND signature is simple. |
| "This is clearly good/bad" | Always run the systematic checklist. |
| "Standard pattern = automatically good" | Patterns can be misapplied. Require specific evidence. |
Anti-Rationalization Table
| Rationalization | Counter |
|---|---|
| "I can see this is fine without the checklist" | Stop. The checklist exists because intuition misses structural problems. Run it anyway. |
| "This is too small to review systematically" | Stop. Small modules become core dependencies. Small doesn't mean safe. |
| "I know this codebase well" | Stop. Familiarity breeds blindness. Fresh eyes (the checklist) catch what you've normalized. |
| "The author is senior/experienced" | Stop. Experience doesn't prevent complexity. Apply same rigor regardless of author. |
| "It passes all the tests" | Stop. Tests check behavior, not design. Passing tests ≠ good architecture. |
| "I don't want to be nitpicky" | Stop. Red flags aren't nits. Complexity symptoms are real problems. Flag them. |
| "They'll push back on this feedback" | Stop. Your job is accurate evaluation, not social comfort. Present evidence. |
| "This matches what we already have" | Stop. Consistency with bad patterns spreads problems. Evaluate independently. |
References: aposd-foundations for complexity symptoms