ryanthedev/code-foundations/aposd-reviewing-module-design

Skill: aposd-reviewing-module-design

Review Is Systematic, Not Intuitive

Run the checklist. The checklist exists because intuition misses structural problems.

Unknown unknowns are highest severity. If it's unclear what code/info is needed for changes, flag immediately.

Evaluation Checklist

Use this systematic checklist when reviewing code:

1. Complexity Symptoms (Ch2)

2. Module Depth (Ch4)

Red flag: If understanding the interface isn't much simpler than understanding the implementation, the module is shallow.

3. Information Hiding (Ch5)

4. Layer Abstraction (Ch7)

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)

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

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]

This skill assesses only. To transform what it flagged, hand off to Skill(code-foundations:aposd-simplifying-complexity); if the fix needs a plan, flag it for /code-foundations:plan.

Before Flagging a Problem

Before reporting any red flag, validate:

1. Steel-man check: What's the best argument this design choice is intentional?
2. Intentional shallowness: Is this an adapter, facade, or decorator where thinness is the point?
3. Testing seam: Is this "leakage" actually a legitimate dependency injection point?
4. 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.

Pattern Consistency Check

Balance: Evaluate patterns on merit, but don't create gratuitous inconsistency. The goal is maintainability, not conformance.

When Principles Conflict

CHECKER: run the ID'd checklist Read(${CLAUDE_SKILL_DIR}/checklists.md) against the code. CHECKER output: the ID'd checklist is the item set (the tables in this SKILL.md explain the items, they are not counted twice). Report each VIOLATION and WARNING as | Item | Status | Evidence | Location |; do not list passing or non-applicable items — close with one line N items checked, M pass, K n/a. The Evaluation Output Format above is the assessment that wraps that report.