diff --git a/backend/.claude/CODE_REVIEW_GAME_ENGINE.md b/backend/.claude/CODE_REVIEW_GAME_ENGINE.md index 9902ec5..0b40d05 100644 --- a/backend/.claude/CODE_REVIEW_GAME_ENGINE.md +++ b/backend/.claude/CODE_REVIEW_GAME_ENGINE.md @@ -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 diff --git a/backend/app/core/game_engine.py b/backend/app/core/game_engine.py index f3a67e4..ca4f658 100644 --- a/backend/app/core/game_engine.py +++ b/backend/app/core/game_engine.py @@ -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: