refactor: extract phase-specific helpers from get_state() (closes #453)#461
refactor: extract phase-specific helpers from get_state() (closes #453)#461
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the get_state method by delegating phase-specific state construction to new private methods: _state_playing, _state_reveal, and _state_end. This change improves the modularity of the game state logic. Reviewers suggested optimizing the _state_end method by caching terminal calculations and recommended using a dictionary comprehension to streamline the extraction of localized song metadata.
| def _state_end(self) -> dict[str, Any]: | ||
| """Return END phase-specific state fragment.""" | ||
| fragment: dict[str, Any] = { | ||
| # Final leaderboard with all player stats (Story 5.6) | ||
| "leaderboard": self.get_final_leaderboard(), | ||
| "game_stats": { | ||
| "total_rounds": self.round, | ||
| "total_players": len(self.players), | ||
| }, | ||
| # Superlatives - fun awards (Story 15.2) | ||
| "superlatives": self.calculate_superlatives(), | ||
| # Issue #75: Game highlights reel | ||
| "highlights": self.highlights_tracker.to_dict(), | ||
| # Issue #120: Shareable result cards | ||
| "share_data": build_share_data(self), | ||
| } | ||
| # Include winner info | ||
| if self.players: | ||
| winner = max(self.players.values(), key=lambda p: p.score) | ||
| fragment["winner"] = {"name": winner.name, "score": winner.score} | ||
| # Game performance comparison for end screen (Story 14.4 AC5, AC6) | ||
| game_performance = self.get_game_performance() | ||
| if game_performance: | ||
| fragment["game_performance"] = game_performance | ||
| return fragment |
There was a problem hiding this comment.
The _state_end method performs several expensive calculations every time it is called, including get_final_leaderboard(), calculate_superlatives(), and build_share_data(self). Since the END phase is terminal and these values do not change once the game is over, they should be calculated once and cached to avoid redundant processing during state broadcasts or client refreshes.
| "fun_fact": self.current_song.get("fun_fact", ""), | ||
| "fun_fact_de": self.current_song.get("fun_fact_de", ""), | ||
| "fun_fact_es": self.current_song.get("fun_fact_es", ""), | ||
| "fun_fact_fr": self.current_song.get("fun_fact_fr", ""), | ||
| "fun_fact_nl": self.current_song.get("fun_fact_nl", ""), |
There was a problem hiding this comment.
The extraction of fun fact translations is repetitive. This can be simplified using a dictionary comprehension to improve maintainability and reduce boilerplate.
| "fun_fact": self.current_song.get("fun_fact", ""), | |
| "fun_fact_de": self.current_song.get("fun_fact_de", ""), | |
| "fun_fact_es": self.current_song.get("fun_fact_es", ""), | |
| "fun_fact_fr": self.current_song.get("fun_fact_fr", ""), | |
| "fun_fact_nl": self.current_song.get("fun_fact_nl", ""), | |
| **{k: self.current_song.get(k, "") for k in ["fun_fact", "fun_fact_de", "fun_fact_es", "fun_fact_fr", "fun_fact_nl"]}, |
mholzi
left a comment
mholzi
left a comment
There was a problem hiding this comment.
Review: Extract phase-specific helpers from get_state() (#453)
Verdict: LGTM ✅
Checked the diff against main across four dimensions:
1. Logic split — no data lost or duplicated
Each helper (_state_playing, _state_reveal, _state_end) extracts exactly the keys from its original elif block. Every key present in the old inline code appears exactly once in the corresponding helper. No keys were dropped or duplicated across helpers.
2. state.update() merge — correct
The base state dict includes common fields (game_id, phase, player_count, players, language, etc.). Each helper returns a fragment dict that update() merges in. Notably, _state_reveal() returns a "players" key (get_reveal_players_state()) that overwrites the base "players" (get_players_state()) — this matches the original behavior where state["players"] was reassigned directly in the REVEAL block.
3. Phase boundaries — correct
- LOBBY: inline
join_url(unchanged) - PLAYING:
_state_playing()— deadline, songs_remaining, submission tracking, song without year, challenges hidden - REVEAL:
_state_reveal()— song with year + fun facts, reveal players, analytics, difficulty, challenges revealed, early_reveal flag - PAUSED: inline
pause_reason(unchanged) - END:
_state_end()— final leaderboard, game stats, winner, superlatives, highlights, share data
No overlap, no gaps.
4. No state mutation or side effects moved
All three helpers are pure reads — they call getters, access attributes, and do computations but never modify self. No side effects were introduced or relocated.
Clean refactor, get_state() reads much better now.
🤖 Generated with Claude Code
1 participant
Fixes #453 — extracts
_state_playing(),_state_reveal(), and_state_end()helpers from the 142-lineget_state()method.