refactor: centralise song dict construction via _build_song_dict() (closes #454)#462
refactor: centralise song dict construction via _build_song_dict() (closes #454)#462
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized helper method, _build_song_dict, to reduce code duplication when extracting song metadata for different game phases. The review feedback suggests improving the method's modularity and robustness by passing the song data as an explicit argument, marking it as a static method, and using a loop to simplify the assignment of localized fun fact fields.
| def _build_song_dict(self, include_reveal: bool = False) -> dict: | ||
| """Build the song info dict from current_song. | ||
|
|
||
| Args: | ||
| include_reveal: If True, include year and fun_fact fields | ||
| shown only during REVEAL phase. | ||
| """ | ||
| song = self.current_song | ||
| result = { | ||
| "artist": song.get("artist", "Unknown"), | ||
| "title": song.get("title", "Unknown"), | ||
| "album_art": song.get( | ||
| "album_art", "/beatify/static/img/no-artwork.svg" | ||
| ), | ||
| } | ||
| if include_reveal: | ||
| result["year"] = song.get("year") | ||
| result["fun_fact"] = song.get("fun_fact", "") | ||
| result["fun_fact_de"] = song.get("fun_fact_de", "") | ||
| result["fun_fact_es"] = song.get("fun_fact_es", "") | ||
| result["fun_fact_fr"] = song.get("fun_fact_fr", "") | ||
| result["fun_fact_nl"] = song.get("fun_fact_nl", "") | ||
| return result |
There was a problem hiding this comment.
The centralized helper method _build_song_dict currently relies on the internal state self.current_song. It is more robust and modular to pass the song data as an argument. Additionally, the return type should be more specific (dict[str, Any]) to match the rest of the file's type hinting style, and the repetitive fun_fact field assignments can be simplified with a loop. Since the method doesn't depend on other instance state, it can be marked as a @staticmethod.
@staticmethod
def _build_song_dict(song: dict[str, Any], include_reveal: bool = False) -> dict[str, Any]:
"""Build the song info dict from song data.
Args:
song: The song dictionary to process.
include_reveal: If True, include year and fun_fact fields
shown only during REVEAL phase.
"""
result = {
"artist": song.get("artist", "Unknown"),
"title": song.get("title", "Unknown"),
"album_art": song.get(
"album_art", "/beatify/static/img/no-artwork.svg"
),
}
if include_reveal:
result["year"] = song.get("year")
for field in ["fun_fact", "fun_fact_de", "fun_fact_es", "fun_fact_fr", "fun_fact_nl"]:
result[field] = song.get(field, "")
return result| "album_art", "/beatify/static/img/no-artwork.svg" | ||
| ), | ||
| } | ||
| state["song"] = self._build_song_dict() |
| "fun_fact_fr": self.current_song.get("fun_fact_fr", ""), | ||
| "fun_fact_nl": self.current_song.get("fun_fact_nl", ""), | ||
| } | ||
| state["song"] = self._build_song_dict(include_reveal=True) |
There was a problem hiding this comment.
mholzi
left a comment
There was a problem hiding this comment.
LGTM — clean, correct refactor.
Fields audit:
_build_song_dict()(PLAYING): producesartist,title,album_artwith identical defaults ("Unknown","Unknown","/beatify/static/img/no-artwork.svg") — matches the old inline dict exactly._build_song_dict(include_reveal=True)(REVEAL): addsyear(no default),fun_fact+ 4 locale variants (default"") — matches the old inline dict exactly.- No fields dropped, no fields added, no default changes.
Code quality:
- Helper is well-named and scoped;
include_revealflag is a clear, minimal interface. - Docstring is concise and useful.
- Net -20/+26 lines — the 6-line increase is the helper signature + docstring, well worth the deduplication.
✅ Would approve if this weren't my own PR.
…loses #454) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fixes #454 — extracts
_build_song_dict(include_reveal=False)to eliminate duplicated song dict construction in PLAYING and REVEAL phases.