hockey-manager-2/REVIEW_FINDINGS.md

Code Review Findings

Date: 2025-10-02 Review Type: Major Review (Phase 2-3 completion) Reviewer: Automated review based on REVIEW.md checklist

---

Executive Summary

Overall code quality is good with solid architecture foundations. The codebase shows:

• ✅ Excellent TypeScript strict mode compliance
• ✅ Good file organization following planned structure
• ✅ Clean separation of concerns (entities, systems, config)
• ⚠️ Several DRY violations requiring attention
• ⚠️ Missing utility layer for common operations
• ⚠️ Some magic numbers not yet extracted to constants

Priority Actions:

1. Critical: Extract coordinate conversion utilities (high duplication risk)
2. Important: Create math utility module for distance calculations
3. Important: Extract remaining magic numbers to constants
4. Nice-to-have: Simplify screen coordinate calculations

---

1. Code Organization & Architecture ✅

File Structure ✅

• Files correctly organized by responsibility
• Clear separation: entities/, systems/, config/, game/
• No circular dependencies detected
• Phaser code appropriately isolated

Files reviewed:

• constants.ts (31 lines)
• main.ts (25 lines)
• GameScene.ts (234 lines) ⚠️ Largest file
• Player.ts (211 lines)
• Puck.ts (126 lines)
• Goal.ts (144 lines)
• BehaviorTree.ts (139 lines)

Naming Conventions ✅

• Classes use PascalCase (Player, GameScene, BehaviorTree)
• Files match class names
• Constants use SCREAMING_SNAKE_CASE (RINK_WIDTH, SCALE)
• Variables/functions use camelCase
• Meaningful names throughout

---

2. DRY (Don't Repeat Yourself) ⚠️

Code Duplication Found

CRITICAL: Coordinate Conversion (4 instances)

Pattern duplicated:

// Converting game coords → screen coords
const centerX = (this.scene.game.config.width as number) / 2;
const centerY = (this.scene.game.config.height as number) / 2;
const screenX = centerX + gameX * SCALE;
const screenY = centerY - gameY * SCALE;

Found in:

1. Player.ts:49-50 - Constructor
2. Player.ts:118-123 - setGamePosition()
3. Puck.ts:20-21 - Constructor
4. Puck.ts:79-84 - setGamePosition()

Recommendation: Extract to utility module

// src/utils/coordinates.ts
export class CoordinateUtils {
  static gameToScreen(scene: Phaser.Scene, gameX: number, gameY: number): { x: number; y: number } {
    const centerX = (scene.game.config.width as number) / 2;
    const centerY = (scene.game.config.height as number) / 2;
    return {
      x: centerX + gameX * SCALE,
      y: centerY - gameY * SCALE
    };
  }

  static screenToGame(scene: Phaser.Scene, screenX: number, screenY: number): { x: number; y: number } {
    const centerX = (scene.game.config.width as number) / 2;
    const centerY = (scene.game.config.height as number) / 2;
    return {
      x: (screenX - centerX) / SCALE,
      y: -(screenY - centerY) / SCALE
    };
  }
}

---

IMPORTANT: Distance Calculation (4 instances)

Pattern duplicated:

const dx = x2 - x1;
const dy = y2 - y1;
const distance = Math.sqrt(dx * dx + dy * dy);

Found in:

1. GameScene.ts:174-176 - Puck position with carrier
2. GameScene.ts:199-201 - Puck pickup check
3. Player.ts:158-160 - Movement update
4. BehaviorTree.ts:110-112 - Shot evaluation

Recommendation: Extract to math utility

// src/utils/math.ts
export class MathUtils {
  static distance(x1: number, y1: number, x2: number, y2: number): number {
    const dx = x2 - x1;
    const dy = y2 - y1;
    return Math.sqrt(dx * dx + dy * dy);
  }

  static distanceSquared(x1: number, y1: number, x2: number, y2: number): number {
    const dx = x2 - x1;
    const dy = y2 - y1;
    return dx * dx + dy * dy; // Faster for comparisons
  }
}

---

IMPORTANT: Body Center Position Calculation (2 instances)

Pattern duplicated:

const bodyX = this.body.x + this.body.width / 2;
const bodyY = this.body.y + this.body.height / 2;

Found in:

1. Player.ts:206-207
2. Puck.ts:120-121

Recommendation: Extract to physics utility or base class method

---

Minor: Angle Normalization (2 instances)

Pattern duplicated:

// Normalize to [-PI, PI]
while (angle > Math.PI) angle -= Math.PI * 2;
while (angle < -Math.PI) angle += Math.PI * 2;

Found in:

1. Player.ts:178-179
2. Player.ts:186-187

Recommendation: Extract to math utility

static normalizeAngle(angle: number): number {
  while (angle > Math.PI) angle -= Math.PI * 2;
  while (angle < -Math.PI) angle += Math.PI * 2;
  return angle;
}

---

3. KISS (Keep It Simple) ⚠️

Complexity Issues

GameScene.update() - 56 lines (GameScene.ts:136-192)

The main update loop handles too many responsibilities:

• Puck position updates
• Puck pickup checks
• Goal checks
• Game state building
• Player AI evaluation
• Player movement
• Puck carrier positioning

Recommendation: Extract methods for clarity

update(_time: number, delta: number) {
  this.updatePuck();
  this.updatePlayers(delta);
  this.checkGoals();
}

private updatePuck() { /* ... */ }
private updatePlayers(delta: number) { /* ... */ }
private checkGoals() { /* ... */ }

Player.update() - Good complexity ✅

Movement logic is complex but well-structured and appropriately commented.

---

4. Type Safety ✅

TypeScript Usage ✅

• strict: true enabled in tsconfig.json
• No any types detected (good use of type assertions: as Phaser.Physics.Arcade.Body)
• Interfaces properly defined (PlayerAttributes, GameState, PlayerAction)
• Union types used for fixed values (PlayerPosition, TeamSide, PuckState)
• Return types explicit on public methods
• Optional chaining not needed (no null issues)

Type Definitions ✅

• Shared types in appropriate locations
• Entity state properly typed
• No inline types duplicated

Excellent type safety throughout!

---

5. Performance ✅ ⚠️

Update Loop Performance ⚠️

Issues found:

1. GameScene.update() - forEach loop (GameScene.ts:154-191)

   • ✅ No object allocations in hot loop
   • ⚠️ Creates gameState object every frame (minor)
   • ⚠️ Multiple distance calculations per frame

2. Player.update() (Player.ts:156-210)

   • ✅ Early return when at target
   • ✅ No allocations
   • ✅ Efficient calculations

3. Puck.update() (Puck.ts:115-125)

   • ✅ Minimal overhead
   • ✅ No allocations

Recommendations:

• Consider caching gameState object and mutating properties instead of recreating
• Use distanceSquared for comparison checks (avoid Math.sqrt when possible)

Phaser Best Practices ✅

• Physics bodies properly configured
• Sprites reused (no pooling needed yet - only 1 player)
• Graphics created once in constructors
• No texture reloading

---

6. Constants & Configuration ⚠️

Centralization ⚠️

Constants properly extracted:

• ✅ RINK_LENGTH, RINK_WIDTH, SCALE
• ✅ BLUE_LINE_OFFSET, GOAL_LINE_OFFSET
• ✅ GOAL_WIDTH, GOAL_DEPTH
• ✅ COLOR_* constants
• ✅ FPS, DEBUG
• ✅ PLAYER_ROTATION_SPEED

Magic numbers still in code:

Location	Value	Meaning	Should Be
Player.ts:74	12, 10	Player radius	PLAYER_RADIUS_GOALIE, PLAYER_RADIUS_SKATER
Player.ts:89	12, 10	Player radius (duplicate)	Same as above
Player.ts:194	/ 10	Speed scale factor	SPEED_SCALE_FACTOR or comment explaining
GameScene.ts:46	80, 75	Player attributes	Consider named constants or config file
GameScene.ts:125	4.5	Faceoff circle radius	FACEOFF_CIRCLE_RADIUS
GameScene.ts:129	5	Center dot radius	CENTER_DOT_RADIUS
GameScene.ts:178	0.1	Stop threshold	MOVEMENT_STOP_THRESHOLD
GameScene.ts:184	1.0	Puck carry distance	PUCK_CARRY_DISTANCE
GameScene.ts:204	1.5	Pickup radius	PUCK_PICKUP_RADIUS
GameScene.ts:228	30	Shot speed	SHOT_SPEED
Player.ts:141	0.9	Deceleration rate	GOAL_DECELERATION_RATE
Player.ts:163	0.1	Stop threshold	MOVEMENT_STOP_THRESHOLD (duplicate!)
Puck.ts:42	50	Max puck velocity	MAX_PUCK_VELOCITY
Goal.ts:24	0.3	Post thickness	GOAL_POST_THICKNESS
Goal.ts:27	0.4	Back bar thickness	GOAL_BAR_THICKNESS
BehaviorTree.ts:53	-26, 26	Goal X positions	GOAL_LINE_OFFSET (already exists!)
BehaviorTree.ts:56	-3, 3	Goalie Y clamp	GOAL_WIDTH / 2 or GOALIE_RANGE
BehaviorTree.ts:81	26, -26	Goal X positions	Duplicate of above
BehaviorTree.ts:106	26, -26	Goal X positions	Duplicate again!
BehaviorTree.ts:115	10	Shooting range	SHOOTING_RANGE
BehaviorTree.ts:135	Math.PI / 4	Shooting angle	SHOOTING_ANGLE_THRESHOLD

CRITICAL: Goal X position (±26) is hardcoded 3 times in BehaviorTree.ts but GOAL_LINE_OFFSET constant already exists!

Recommendations

High Priority:

// Add to constants.ts
export const GOAL_LINE_OFFSET = 26; // Already exists - USE IT!

export const PLAYER_RADIUS_GOALIE = 12;
export const PLAYER_RADIUS_SKATER = 10;

export const MOVEMENT_STOP_THRESHOLD = 0.1; // meters
export const PUCK_PICKUP_RADIUS = 1.5; // meters
export const PUCK_CARRY_DISTANCE = 1.0; // meters in front of player
export const SHOT_SPEED = 30; // m/s
export const MAX_PUCK_VELOCITY = 50; // m/s

export const SHOOTING_RANGE = 10; // meters
export const SHOOTING_ANGLE_THRESHOLD = Math.PI / 4; // 45 degrees

---

7. Error Handling ✅

Robustness ✅

• Position validation via setCollideWorldBounds(true)
• Division by zero checks in distance calculations (if (distance > 0))
• Null guards in Goal.checkGoal() (if (!puckBody || !zoneBody) return)
• Angle normalization prevents overflow

Debugging ✅

• Meaningful console logs (${player.id} picked up the puck)
• DEBUG constant for Phaser physics debug view
• Visual debug zones in goals (green translucent)

No issues found.

---

8. Comments & Documentation ✅

Code Comments ✅

• Complex algorithms explained (Player rotation, angle normalization)
• JSDoc on public methods (setGamePosition, setTarget, update)
• Inline comments clarify coordinate systems
• Physics body setup well-commented

Project Documentation ✅

• CLAUDE.md accurately reflects current architecture
• PLAN.md shows phase progress
• NOTES.md exists (not reviewed in this pass)
• README.md exists (assumed correct)

No issues found.

---

9. Testing Readiness ⚠️

Testability ⚠️

• ✅ BehaviorTree has static methods (easy to unit test)
• ✅ Pure logic separated (distance calc, angle calc)
• ⚠️ Game logic tightly coupled to Phaser (Player, Puck extend Phaser classes)
• ⚠️ No dependency injection (scene passed directly)
• ⚠️ Update loops contain state mutation (harder to test)

Recommendations:

• Extract pure game logic to separate classes/functions
• Consider separating rendering from game state
• Add unit tests for BehaviorTree decisions
• Add integration tests for coordinate conversions

---

10. Git Hygiene ✅

Not reviewed in this pass (would require git history inspection).

Assumed clean based on visible code quality.

---

Summary of Issues

Critical (Fix Immediately)

1. Coordinate conversion duplication (4 instances) → Create CoordinateUtils
2. Hardcoded goal positions in BehaviorTree (3 instances) → Use existing GOAL_LINE_OFFSET constant

Important (Fix Soon)

3. Distance calculation duplication (4 instances) → Create MathUtils.distance()
4. 22+ magic numbers not in constants → Extract to constants.ts
5. GameScene.update() too complex → Extract into smaller methods

Nice-to-Have (Improvements)

6. Angle normalization duplication → Extract utility function
7. Body center calculation duplication → Extract method
8. GameState object recreation → Consider object reuse for performance
9. Testability → Decouple game logic from Phaser rendering

---

Refactoring Roadmap

Step 1: Create Utilities (High Priority)

1. Create src/utils/coordinates.ts with conversion functions
2. Create src/utils/math.ts with distance/angle utilities
3. Update all files to use new utilities
4. Test coordinate conversions thoroughly

Step 2: Extract Constants (High Priority)

1. Add all magic numbers to constants.ts
2. Update BehaviorTree to use GOAL_LINE_OFFSET instead of hardcoded 26
3. Replace all numeric literals with named constants
4. Group constants logically (PLAYER_, PUCK_, SHOOTING_*)

Step 3: Simplify GameScene (Medium Priority)

1. Extract updatePuck() method
2. Extract updatePlayers() method
3. Extract checkGoals() method
4. Consider checkPuckPickup() inline or as property

Step 4: Performance Tweaks (Low Priority)

1. Cache gameState object (mutate instead of recreate)
2. Use distanceSquared where comparison only is needed
3. Profile with many players (12+) to identify bottlenecks

Step 5: Testing Setup (Future)

1. Add Jest/Vitest configuration
2. Create unit tests for BehaviorTree
3. Create unit tests for utilities
4. Add integration tests for game flow

---

Conclusion

Overall Grade: B+

The codebase is in excellent shape for this stage of development (Phase 2-3). Architecture is sound, TypeScript usage is exemplary, and code is generally clean and readable.

Key Strengths:

• Solid architecture with clear separation of concerns
• Excellent TypeScript strict mode compliance
• Good performance patterns in hot paths
• Clean, readable code with meaningful names

Key Weaknesses:

• DRY violations in coordinate/math operations (easily fixable)
• Too many magic numbers not yet extracted
• GameScene.update() could be cleaner

Recommendation: Address critical DRY issues and extract magic numbers before proceeding to Phase 4. This will make behavior tree tuning much easier when you need to adjust shooting ranges, speeds, and decision thresholds.

---

Next Review: After Phase 5 (Team offensive positioning system) or when player count increases to 12.