CLAUDE: Extract common resolution logic to _finalize_play method
Issue #5 from code review - resolve_play and resolve_manual_play shared ~70% of their code causing maintenance burden and divergent behavior risk. Changes: - Extracted common finalization logic to new _finalize_play() method - resolve_play reduced from ~150 lines to ~60 lines - resolve_manual_play reduced from ~135 lines to ~60 lines - Single source of truth for: roll tracking, state capture, transaction handling, inning advance, cleanup, and state updates Benefits: - Changes to play finalization only need to be made in one place - Reduced risk of divergent behavior between resolution modes - Better testability and maintainability All 739 unit tests passing (1 flaky probabilistic test occasionally fails). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Cal Corum
parent
72a3b94ce7
commit
b95c5837b0
@ -286,7 +286,12 @@ Some methods seem designed for testing but are public API.
|
||||
- Added optional `session` parameter to `save_play` and `update_game_state` in DatabaseOperations
|
||||
- Wrapped multi-step DB ops in `resolve_play` and `resolve_manual_play` with single transaction
|
||||
- Transaction commits atomically or rolls back entirely on failure
|
||||
- [ ] High issues #5-11 pending
|
||||
- [x] High issue #5 fixed (2025-11-19)
|
||||
- Extracted common logic to `_finalize_play()` method
|
||||
- `resolve_play` reduced from ~150 lines to ~60 lines
|
||||
- `resolve_manual_play` reduced from ~135 lines to ~60 lines
|
||||
- Single source of truth for: roll tracking, state capture, transaction handling, inning advance, cleanup
|
||||
- [ ] High issues #6-11 pending
|
||||
- [ ] Medium issues addressed
|
||||
- [ ] Low issues addressed
|
||||
|
||||
|
||||
@ -404,6 +404,118 @@ class GameEngine:
|
||||
logger.warning(f"Offensive decision timeout for game {state.game_id}, using default")
|
||||
return OffensiveDecision() # All defaults
|
||||
|
||||
async def _finalize_play(
|
||||
self,
|
||||
state: GameState,
|
||||
result: PlayResult,
|
||||
ab_roll,
|
||||
log_suffix: str = ""
|
||||
) -> None:
|
||||
"""
|
||||
Common finalization logic for both resolve_play and resolve_manual_play.
|
||||
|
||||
Handles:
|
||||
- Roll tracking for batch saving
|
||||
- State capture and result application
|
||||
- Transaction with DB operations (save play, update state, advance inning)
|
||||
- Batch save rolls at inning boundary
|
||||
- Prepare next play or cleanup on completion
|
||||
- Clear decisions and update state
|
||||
|
||||
Args:
|
||||
state: Current game state (modified in place)
|
||||
result: Play result to apply
|
||||
ab_roll: The dice roll to track
|
||||
log_suffix: Optional suffix for log message (e.g., " (hit to SS)")
|
||||
"""
|
||||
game_id = state.game_id
|
||||
|
||||
# Track roll for batch saving at end of inning
|
||||
if game_id not in self._rolls_this_inning:
|
||||
self._rolls_this_inning[game_id] = []
|
||||
self._rolls_this_inning[game_id].append(ab_roll)
|
||||
|
||||
# Capture state before applying result
|
||||
state_before = {
|
||||
'inning': state.inning,
|
||||
'half': state.half,
|
||||
'home_score': state.home_score,
|
||||
'away_score': state.away_score,
|
||||
'status': state.status
|
||||
}
|
||||
|
||||
# Apply result to state (outs, score, runners) - before transaction
|
||||
self._apply_play_result(state, result)
|
||||
|
||||
# Database operations in single transaction
|
||||
async with AsyncSessionLocal() as session:
|
||||
try:
|
||||
# Save play to DB (uses snapshot from GameState)
|
||||
await self._save_play_to_db(state, result, session=session)
|
||||
|
||||
# Update game state in DB only if something changed
|
||||
if (state.inning != state_before['inning'] or
|
||||
state.half != state_before['half'] or
|
||||
state.home_score != state_before['home_score'] or
|
||||
state.away_score != state_before['away_score'] or
|
||||
state.status != state_before['status']):
|
||||
|
||||
await self.db_ops.update_game_state(
|
||||
game_id=state.game_id,
|
||||
inning=state.inning,
|
||||
half=state.half,
|
||||
home_score=state.home_score,
|
||||
away_score=state.away_score,
|
||||
status=state.status,
|
||||
session=session
|
||||
)
|
||||
logger.info("Updated game state in DB - score/inning/status changed")
|
||||
else:
|
||||
logger.debug("Skipped game state update - no changes to persist")
|
||||
|
||||
# Check for inning change
|
||||
if state.outs >= 3:
|
||||
await self._advance_inning(state, game_id)
|
||||
# Update DB again after inning change
|
||||
await self.db_ops.update_game_state(
|
||||
game_id=state.game_id,
|
||||
inning=state.inning,
|
||||
half=state.half,
|
||||
home_score=state.home_score,
|
||||
away_score=state.away_score,
|
||||
status=state.status,
|
||||
session=session
|
||||
)
|
||||
|
||||
# Commit entire transaction
|
||||
await session.commit()
|
||||
logger.debug("Committed play transaction successfully")
|
||||
|
||||
except Exception as e:
|
||||
await session.rollback()
|
||||
logger.error(f"Transaction failed, rolled back: {e}")
|
||||
raise
|
||||
|
||||
# Batch save rolls at half-inning boundary (separate transaction - audit data)
|
||||
if state.outs >= 3:
|
||||
await self._batch_save_inning_rolls(game_id)
|
||||
|
||||
# Prepare next play or clean up if game completed
|
||||
if state.status == "active":
|
||||
await self._prepare_next_play(state)
|
||||
elif state.status == "completed":
|
||||
# Clean up per-game resources to prevent memory leaks
|
||||
self._cleanup_game_resources(game_id)
|
||||
|
||||
# Clear decisions for next play
|
||||
state.decisions_this_play = {}
|
||||
state.pending_decision = "defensive"
|
||||
|
||||
# Update in-memory state
|
||||
state_manager.update_state(game_id, state)
|
||||
|
||||
logger.info(f"Resolved play {state.play_count} for game {game_id}: {result.description}{log_suffix}")
|
||||
|
||||
async def resolve_play(
|
||||
self,
|
||||
game_id: UUID,
|
||||
@ -413,15 +525,7 @@ class GameEngine:
|
||||
xcheck_error: Optional[str] = None
|
||||
) -> PlayResult:
|
||||
"""
|
||||
Resolve the current play with dice roll
|
||||
|
||||
Explicit orchestration sequence:
|
||||
1. Resolve play with dice rolls
|
||||
2. Save play to DB (uses snapshot from GameState)
|
||||
3. Apply result to state (outs, score, runners)
|
||||
4. Update game state in DB
|
||||
5. Check for inning change (outs >= 3)
|
||||
6. Prepare next play (always last step)
|
||||
Resolve the current play with dice roll (testing/forced outcome method).
|
||||
|
||||
Args:
|
||||
game_id: Game to resolve
|
||||
@ -443,7 +547,6 @@ class GameEngine:
|
||||
defensive_decision = DefensiveDecision(**state.decisions_this_play.get('defensive', {}))
|
||||
offensive_decision = OffensiveDecision(**state.decisions_this_play.get('offensive', {}))
|
||||
|
||||
# STEP 1: Resolve play
|
||||
# Create resolver for this game's league and mode
|
||||
resolver = PlayResolver(league_id=state.league_id, auto_mode=state.auto_mode, state_manager=state_manager)
|
||||
|
||||
@ -471,91 +574,9 @@ class GameEngine:
|
||||
forced_xcheck_error=xcheck_error
|
||||
)
|
||||
|
||||
# Track roll for batch saving at end of inning
|
||||
if game_id not in self._rolls_this_inning:
|
||||
self._rolls_this_inning[game_id] = []
|
||||
self._rolls_this_inning[game_id].append(result.ab_roll)
|
||||
# Finalize the play (common logic)
|
||||
await self._finalize_play(state, result, result.ab_roll)
|
||||
|
||||
# Capture state before applying result
|
||||
state_before = {
|
||||
'inning': state.inning,
|
||||
'half': state.half,
|
||||
'home_score': state.home_score,
|
||||
'away_score': state.away_score,
|
||||
'status': state.status
|
||||
}
|
||||
|
||||
# STEP 3: Apply result to state (outs, score, runners) - before transaction
|
||||
self._apply_play_result(state, result)
|
||||
|
||||
# STEP 2, 4, 5: Database operations in single transaction
|
||||
async with AsyncSessionLocal() as session:
|
||||
try:
|
||||
# STEP 2: Save play to DB (uses snapshot from GameState)
|
||||
await self._save_play_to_db(state, result, session=session)
|
||||
|
||||
# STEP 4: Update game state in DB only if something changed
|
||||
if (state.inning != state_before['inning'] or
|
||||
state.half != state_before['half'] or
|
||||
state.home_score != state_before['home_score'] or
|
||||
state.away_score != state_before['away_score'] or
|
||||
state.status != state_before['status']):
|
||||
|
||||
await self.db_ops.update_game_state(
|
||||
game_id=state.game_id,
|
||||
inning=state.inning,
|
||||
half=state.half,
|
||||
home_score=state.home_score,
|
||||
away_score=state.away_score,
|
||||
status=state.status,
|
||||
session=session
|
||||
)
|
||||
logger.info("Updated game state in DB - score/inning/status changed")
|
||||
else:
|
||||
logger.debug("Skipped game state update - no changes to persist")
|
||||
|
||||
# STEP 5: Check for inning change
|
||||
if state.outs >= 3:
|
||||
await self._advance_inning(state, game_id)
|
||||
# Update DB again after inning change
|
||||
await self.db_ops.update_game_state(
|
||||
game_id=state.game_id,
|
||||
inning=state.inning,
|
||||
half=state.half,
|
||||
home_score=state.home_score,
|
||||
away_score=state.away_score,
|
||||
status=state.status,
|
||||
session=session
|
||||
)
|
||||
|
||||
# Commit entire transaction
|
||||
await session.commit()
|
||||
logger.debug("Committed play transaction successfully")
|
||||
|
||||
except Exception as e:
|
||||
await session.rollback()
|
||||
logger.error(f"Transaction failed, rolled back: {e}")
|
||||
raise
|
||||
|
||||
# Batch save rolls at half-inning boundary (separate transaction - audit data)
|
||||
if state.outs >= 3:
|
||||
await self._batch_save_inning_rolls(game_id)
|
||||
|
||||
# STEP 6: Prepare next play or clean up if game completed
|
||||
if state.status == "active":
|
||||
await self._prepare_next_play(state)
|
||||
elif state.status == "completed":
|
||||
# Clean up per-game resources to prevent memory leaks
|
||||
self._cleanup_game_resources(game_id)
|
||||
|
||||
# Clear decisions for next play
|
||||
state.decisions_this_play = {}
|
||||
state.pending_decision = "defensive"
|
||||
|
||||
# Update in-memory state
|
||||
state_manager.update_state(game_id, state)
|
||||
|
||||
logger.info(f"Resolved play {state.play_count} for game {game_id}: {result.description}")
|
||||
return result
|
||||
|
||||
async def resolve_manual_play(
|
||||
@ -574,14 +595,6 @@ class GameEngine:
|
||||
3. Players submit the outcome they see
|
||||
4. Server validates and processes with the provided outcome
|
||||
|
||||
Orchestration sequence (same as resolve_play):
|
||||
1. Resolve play with manual outcome (uses ab_roll for audit trail)
|
||||
2. Save play to DB
|
||||
3. Apply result to state
|
||||
4. Update game state in DB
|
||||
5. Check for inning change
|
||||
6. Prepare next play
|
||||
|
||||
Args:
|
||||
game_id: Game to resolve
|
||||
ab_roll: The dice roll (server-rolled for fairness)
|
||||
@ -611,7 +624,6 @@ class GameEngine:
|
||||
defensive_decision = DefensiveDecision(**state.decisions_this_play.get('defensive', {}))
|
||||
offensive_decision = OffensiveDecision(**state.decisions_this_play.get('offensive', {}))
|
||||
|
||||
# STEP 1: Resolve play with manual outcome
|
||||
# Create resolver for this game's league and mode
|
||||
resolver = PlayResolver(league_id=state.league_id, auto_mode=state.auto_mode, state_manager=state_manager)
|
||||
|
||||
@ -625,94 +637,10 @@ class GameEngine:
|
||||
ab_roll=ab_roll
|
||||
)
|
||||
|
||||
# Track roll for batch saving at end of inning (same as auto mode)
|
||||
if game_id not in self._rolls_this_inning:
|
||||
self._rolls_this_inning[game_id] = []
|
||||
self._rolls_this_inning[game_id].append(ab_roll)
|
||||
# Finalize the play (common logic)
|
||||
log_suffix = f" (hit to {hit_location})" if hit_location else ""
|
||||
await self._finalize_play(state, result, ab_roll, log_suffix)
|
||||
|
||||
# Capture state before applying result
|
||||
state_before = {
|
||||
'inning': state.inning,
|
||||
'half': state.half,
|
||||
'home_score': state.home_score,
|
||||
'away_score': state.away_score,
|
||||
'status': state.status
|
||||
}
|
||||
|
||||
# STEP 3: Apply result to state - before transaction
|
||||
self._apply_play_result(state, result)
|
||||
|
||||
# STEP 2, 4, 5: Database operations in single transaction
|
||||
async with AsyncSessionLocal() as session:
|
||||
try:
|
||||
# STEP 2: Save play to DB
|
||||
await self._save_play_to_db(state, result, session=session)
|
||||
|
||||
# STEP 4: Update game state in DB only if something changed
|
||||
if (state.inning != state_before['inning'] or
|
||||
state.half != state_before['half'] or
|
||||
state.home_score != state_before['home_score'] or
|
||||
state.away_score != state_before['away_score'] or
|
||||
state.status != state_before['status']):
|
||||
|
||||
await self.db_ops.update_game_state(
|
||||
game_id=state.game_id,
|
||||
inning=state.inning,
|
||||
half=state.half,
|
||||
home_score=state.home_score,
|
||||
away_score=state.away_score,
|
||||
status=state.status,
|
||||
session=session
|
||||
)
|
||||
logger.info("Updated game state in DB - score/inning/status changed")
|
||||
else:
|
||||
logger.debug("Skipped game state update - no changes to persist")
|
||||
|
||||
# STEP 5: Check for inning change
|
||||
if state.outs >= 3:
|
||||
await self._advance_inning(state, game_id)
|
||||
# Update DB again after inning change
|
||||
await self.db_ops.update_game_state(
|
||||
game_id=state.game_id,
|
||||
inning=state.inning,
|
||||
half=state.half,
|
||||
home_score=state.home_score,
|
||||
away_score=state.away_score,
|
||||
status=state.status,
|
||||
session=session
|
||||
)
|
||||
|
||||
# Commit entire transaction
|
||||
await session.commit()
|
||||
logger.debug("Committed manual play transaction successfully")
|
||||
|
||||
except Exception as e:
|
||||
await session.rollback()
|
||||
logger.error(f"Manual play transaction failed, rolled back: {e}")
|
||||
raise
|
||||
|
||||
# Batch save rolls at half-inning boundary (separate transaction - audit data)
|
||||
if state.outs >= 3:
|
||||
await self._batch_save_inning_rolls(game_id)
|
||||
|
||||
# STEP 6: Prepare next play or clean up if game completed
|
||||
if state.status == "active":
|
||||
await self._prepare_next_play(state)
|
||||
elif state.status == "completed":
|
||||
# Clean up per-game resources to prevent memory leaks
|
||||
self._cleanup_game_resources(game_id)
|
||||
|
||||
# Clear decisions for next play
|
||||
state.decisions_this_play = {}
|
||||
state.pending_decision = "defensive"
|
||||
|
||||
# Update in-memory state
|
||||
state_manager.update_state(game_id, state)
|
||||
|
||||
logger.info(
|
||||
f"Resolved manual play {state.play_count} for game {game_id}: "
|
||||
f"{result.description}" + (f" (hit to {hit_location})" if hit_location else "")
|
||||
)
|
||||
return result
|
||||
|
||||
def _apply_play_result(self, state: GameState, result: PlayResult) -> None:
|
||||
|
||||
Loading…
Reference in New Issue
Block a user