...
This commit is contained in:
Pierre Wessman
parent
8ec240333c
commit
40d07ee68c
9
.claude/settings.local.json
Normal file
9
.claude/settings.local.json
Normal file
@ -0,0 +1,9 @@
|
||||
{
|
||||
"permissions": {
|
||||
"allow": [
|
||||
"Bash(find:*)"
|
||||
],
|
||||
"deny": [],
|
||||
"ask": []
|
||||
}
|
||||
}
|
||||
336
REVIEW.md
Normal file
336
REVIEW.md
Normal file
@ -0,0 +1,336 @@
|
||||
# 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.
|
||||
@ -25,6 +25,7 @@ export const COLOR_GOAL_CREASE = 0x87ceeb;
|
||||
|
||||
// Game settings
|
||||
export const FPS = 60;
|
||||
export const DEBUG = true;
|
||||
|
||||
// Player movement
|
||||
export const PLAYER_ROTATION_SPEED = 1; // radians per second
|
||||
|
||||
@ -1,6 +1,6 @@
|
||||
import Phaser from 'phaser';
|
||||
import { GameScene } from './GameScene';
|
||||
import { RINK_LENGTH, RINK_WIDTH, SCALE, FPS } from '../config/constants';
|
||||
import { RINK_LENGTH, RINK_WIDTH, SCALE, FPS, DEBUG } from '../config/constants';
|
||||
|
||||
const config: Phaser.Types.Core.GameConfig = {
|
||||
type: Phaser.AUTO,
|
||||
@ -12,7 +12,7 @@ const config: Phaser.Types.Core.GameConfig = {
|
||||
default: 'arcade',
|
||||
arcade: {
|
||||
gravity: { x: 0, y: 0 },
|
||||
debug: true
|
||||
debug: DEBUG
|
||||
}
|
||||
},
|
||||
fps: {
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user