hockey-manager-2/REVIEW.md

REVIEW.md

This document provides a systematic checklist for conducting periodic code reviews of the hockey-manager-2 codebase. Use this to maintain code quality, consistency, and adherence to best practices.

Review Frequency

• Minor Review: After completing each phase (see PLAN.md)
• Major Review: Every 2-3 phases or when accumulating significant technical debt
• Pre-Release Review: Before any major release or milestone

Review Checklist

1. Code Organization & Architecture

File Structure

• Files are in correct directories according to their responsibility
• No circular dependencies between modules
• Clear separation of concerns (entities, systems, config, scenes)
• Phaser-specific code isolated from game logic where possible

Naming Conventions

• Classes use PascalCase (Player, BehaviorTree)
• Files match class names (Player.ts, GameScene.ts)
• Constants use SCREAMING_SNAKE_CASE (RINK_WIDTH, TARGET_FPS)
• Variables and functions use camelCase (currentPosition, calculateDistance())
• Meaningful, descriptive names (avoid temp, data, handler without context)

2. DRY (Don't Repeat Yourself)

Code Duplication

• No duplicate logic across files (extract to shared utilities)
• No duplicate constants (all magic numbers in constants.ts)
• No duplicate type definitions (centralize in types file)
• Similar entity behaviors extracted to base classes or systems

Common Patterns to Extract

• Distance calculations → utility function
• Position validation/clamping → utility function
• Phaser object creation → factory functions
• Coordinate conversions (meters ↔ pixels) → utility functions

3. KISS (Keep It Simple, Stupid)

Simplicity Checks

• Functions do one thing and do it well
• No overly clever solutions that sacrifice readability
• Complex algorithms have explanatory comments
• Prefer explicit over implicit behavior
• Avoid premature optimization

Function Complexity

• Functions < 50 lines (guideline, not hard rule)
• Nested conditionals < 3 levels deep
• Function parameters < 5 (consider config objects for more)
• No "god functions" that do too much

4. Type Safety

TypeScript Usage

• No any types (use unknown if truly needed)
• Interfaces defined for complex objects
• Return types explicitly declared on public methods
• Proper null/undefined handling with optional chaining
• Enums or union types for fixed sets of values

Type Definitions

• Shared types in dedicated types file
• No inline type definitions used multiple times
• Entity state properly typed (position, velocity, attributes)

5. Performance

Hot Path Optimization

• update() loops avoid unnecessary allocations
• Object pooling for frequently created/destroyed objects (bullets, particles)
• Minimize garbage collection pressure (reuse objects where possible)
• Expensive calculations cached when appropriate

Phaser Best Practices

• Sprites reused via object pools
• Physics bodies properly cleaned up
• Textures loaded once, referenced many times
• Update loops only process active entities

6. Constants & Configuration

Centralization

• All magic numbers extracted to constants.ts
• Related constants grouped logically (RINK_, PLAYER_, PUCK_*)
• Constants documented with units (meters, pixels, seconds)
• Derived constants calculated from base constants

Tunable Values

• Game balance values clearly marked for tuning
• Physics values (speed, acceleration) easy to adjust
• AI behavior probabilities configurable
• Debug flags for visualization toggles

7. Error Handling

Robustness

• Invalid positions clamped/validated
• Division by zero checks
• Null/undefined guards on external data
• Graceful degradation for missing resources

Debugging

• Meaningful console warnings for unexpected states
• Debug visualization available (toggle in constants)
• Assert-like checks in development builds

8. Comments & Documentation

Code Comments

• Complex algorithms explained
• Non-obvious decisions justified
• Public APIs documented (JSDoc style)
• TODOs tracked and dated

Project Documentation

• CLAUDE.md reflects current architecture
• PLAN.md updated with completion status
• NOTES.md captures important design decisions
• README accurate for development setup

9. Testing Readiness

Testability

• Pure functions separated from side effects
• Game logic decoupled from Phaser where possible
• Dependency injection for systems (not hardcoded singletons)
• State machines/behavior trees unit-testable

Manual Testing

• Visual debug modes available
• Hot reload works correctly
• No console errors in normal operation
• Performance profiling points identified

10. Git Hygiene

Commit Quality

• Logical, atomic commits
• Clear commit messages
• No commented-out code blocks committed
• No debug console.logs in production code

Branch Management

• Dead branches cleaned up
• Feature branches merged or rebased
• Master/main branch stable

Review Process

Before Starting Review

1. Create a review branch (optional but recommended)

   git checkout -b review/phase-N
   

2. Run the build

   pnpm run build
   

   Ensure no errors before starting review.

3. Document findings in a REVIEW_NOTES.md or GitHub issues

During Review

1. Scan each section systematically using checklist above

2. Note issues with file path and line number

3. Categorize by priority:

   • Critical: Bugs, type safety issues, performance problems
   • Important: DRY violations, architectural issues
   • Nice-to-have: Better names, additional comments

4. Create refactor tasks for important findings

After Review

1. Prioritize fixes (critical → important → nice-to-have)
2. Refactor incrementally (small, testable changes)
3. Update documentation if architecture changed
4. Commit refactors separately from feature work

Specific Code Smells to Watch For

Hockey Manager Specific

• Hardcoded positions: All positions should derive from constants
• Pixel/meter confusion: Ensure conversions are correct and consistent
• Missing coordinate validation: Positions should stay within rink bounds
• Attribute magic numbers: Player attributes (0-20 scale) should be typed
• Decision probabilities: AI chances should be in constants, not inline

Phaser Specific

• Memory leaks: Sprites/physics bodies not destroyed
• Update loop bloat: Too much computation in GameScene.update()
• Texture management: Multiple loads of same texture
• Scale confusion: Mixing world coordinates with screen coordinates

TypeScript Specific

• Type assertions: Excessive use of as keyword
• Implicit any: Missing type annotations
• Non-null assertions: Overuse of ! operator
• Enum misuse: Strings where enums would be clearer

Refactoring Patterns

Extract Utility Function

Before:

const dist = Math.sqrt(
  Math.pow(x2 - x1, 2) + Math.pow(y2 - y1, 2)
);

After:

// In utils/math.ts
export function distance(x1: number, y1: number, x2: number, y2: number): number {
  return Math.sqrt(Math.pow(x2 - x1, 2) + Math.pow(y2 - y1, 2));
}

Extract Constants

Before:

if (player.speed > 8.5) { // max skating speed

After:

// In constants.ts
export const MAX_SKATING_SPEED = 8.5; // m/s

// In code
if (player.speed > MAX_SKATING_SPEED) {

Extract Type

Before:

function updatePlayer(data: { x: number; y: number; team: string }) {

After:

// In types.ts
interface Position {
  x: number;
  y: number;
}

type Team = 'home' | 'away';

interface PlayerData {
  position: Position;
  team: Team;
}

function updatePlayer(data: PlayerData) {

Simplify Nested Conditionals

Before:

if (hasPuck) {
  if (inShootingRange) {
    if (skill > 15) {
      shoot();
    }
  }
}

After:

const canShoot = hasPuck && inShootingRange && skill > 15;
if (canShoot) {
  shoot();
}

Tools & Commands

Find Duplicated Code

# Search for duplicate function patterns
grep -r "function calculateDistance" src/

# Find magic numbers (numbers not in constants)
grep -rE "\b[0-9]+\.?[0-9]*\b" src/ --exclude-dir=config

Count Lines Per File

# Find large files that might need splitting
find src -name "*.ts" -exec wc -l {} + | sort -rn

TypeScript Strict Checks

Ensure tsconfig.json has:

{
  "compilerOptions": {
    "strict": true,
    "noImplicitAny": true,
    "strictNullChecks": true
  }
}

Success Criteria

A successful review results in:

• ✅ All critical issues resolved
• ✅ No obvious code duplication
• ✅ Constants properly centralized
• ✅ Type safety throughout
• ✅ Documentation reflects current state
• ✅ Build passes without warnings
• ✅ Code runs smoothly in dev mode

---

Remember: The goal is maintainable, readable, and performant code. Don't sacrifice clarity for cleverness. When in doubt, be explicit.