# 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](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](src/config/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](src/config/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](CLAUDE.md) reflects current architecture
- [ ] [PLAN.md](PLAN.md) updated with completion status
- [ ] [NOTES.md](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)
   ```bash
   git checkout -b review/phase-N
   ```

2. **Run the build**
   ```bash
   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:**
```typescript
const dist = Math.sqrt(
  Math.pow(x2 - x1, 2) + Math.pow(y2 - y1, 2)
);
```

**After:**
```typescript
// 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:**
```typescript
if (player.speed > 8.5) { // max skating speed
```

**After:**
```typescript
// In constants.ts
export const MAX_SKATING_SPEED = 8.5; // m/s

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

### Extract Type

**Before:**
```typescript
function updatePlayer(data: { x: number; y: number; team: string }) {
```

**After:**
```typescript
// 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:**
```typescript
if (hasPuck) {
  if (inShootingRange) {
    if (skill > 15) {
      shoot();
    }
  }
}
```

**After:**
```typescript
const canShoot = hasPuck && inShootingRange && skill > 15;
if (canShoot) {
  shoot();
}
```

## Tools & Commands

### Find Duplicated Code
```bash
# 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
```bash
# Find large files that might need splitting
find src -name "*.ts" -exec wc -l {} + | sort -rn
```

### TypeScript Strict Checks
Ensure `tsconfig.json` has:
```json
{
  "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.