Add generics for settings - #2
Conversation
📝 WalkthroughWalkthroughThis PR parameterizes the core Game abstraction as Game<S, D : Dataset>, replaces Map-based settings with strongly-typed settings classes, updates dataset types across games, and widens public APIs to use Game<,> across registry and UI layers. Changes
Sequence Diagram(s)(Skipped — changes are type/generalization and do not introduce new multi-component control flow requiring sequence diagrams.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@composeApp/src/commonMain/kotlin/de/benkralex/partygames/datasets/loadDatasets.kt`:
- Around line 42-52: In parseDataset, the Napier.i("Loaded dataset...") is
emitted before checking duplicates; change the flow so you only log the success
message after confirming and adding the dataset to game.datasets (i.e., move the
info log into the else block where game.datasets.add(dataset) and return dataset
occur), and keep or refine the Napier.w warning that references dataset.uid when
the dataset already exists; keep the Napier.e log for the parse failure branch
unchanged.
🧹 Nitpick comments (4)
composeApp/src/commonMain/kotlin/de/benkralex/partygames/datasets/loadDatasets.kt (1)
42-42: Consider returningD?instead ofDataset?for better type safety.The function signature uses generic
Dbut returnsDataset?, losing the type information. While this may not cause runtime issues since the result isn't used in a type-specific way currently, returningD?would be more consistent with the generic design.Proposed change
-fun <D : Dataset> parseDataset(json: JsonObject, game: Game<*, D>, gameId: String): Dataset? { +fun <D : Dataset> parseDataset(json: JsonObject, game: Game<*, D>, gameId: String): D? {composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/truthOrDare/domain/TruthOrDare.kt (1)
50-50: Consider movingTruthOrDareSettingsto a separate file.For consistency with other game modules (e.g.,
ImpostorSettings,FindLiarSettingslikely have their own files), consider moving this data class to a dedicatedTruthOrDareSettings.ktfile. This improves discoverability and keeps the domain model organized.composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/findLiar/presentation/FindLiarPlayViewModel.kt (1)
19-26: Consider failing fast when settings are missing or invalid.Defaulting to empty lists/
1can hide misconfiguration;initNewRoundlater callsplayers.random()and loops untilliarCountis reached, which can throw or hang when players are empty orliarCountexceedsplayers.size. A small guard keeps this safe.Proposed guard in
initNewRoundfun initNewRound() { if (game == null) { Napier.e("Game is not initialized yet") return } + if (players.isEmpty()) { + Napier.e("No players configured") + return + } + if (liarCount !in 1..players.size) { + Napier.e("Invalid liar count: $liarCount") + return + } if (questionPairs.none { !playedQuestions.contains(it) }) { Napier.e("No question pairs available for the selected topics") return }composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/impostor/domain/Impostor.kt (1)
80-87: Consider typinghintmore narrowly.
hint: Any?reintroduces the need for casts and weakens the type-safety goal of this PR. If the hint has a known domain, a dedicated type (e.g.,TranslatableString?or a sealed class) keeps the API safer.
| fun <D : Dataset> parseDataset(json: JsonObject, game: Game<*, D>, gameId: String): Dataset? { | ||
| game.parseData(json)?.let { dataset -> | ||
| Napier.i("Loaded dataset for game $gameId") | ||
| if (game.datasets.any { it.uid == dataset.uid }) { | ||
| Napier.w("Dataset with uid ${dataset.uid} already exists for game $gameId, skipping") | ||
| } else { | ||
| game.datasets.add(dataset) | ||
| return dataset | ||
| } | ||
| } ?: Napier.e("Failed to parse dataset for game $gameId, skipping") | ||
| return null |
There was a problem hiding this comment.
Misleading log message when dataset is a duplicate.
The "Loaded dataset" message is logged before checking for duplicates, which means it will report success even when the dataset is skipped due to duplication. Consider restructuring the logging:
Proposed fix
fun <D : Dataset> parseDataset(json: JsonObject, game: Game<*, D>, gameId: String): Dataset? {
game.parseData(json)?.let { dataset ->
- Napier.i("Loaded dataset for game $gameId")
if (game.datasets.any { it.uid == dataset.uid }) {
Napier.w("Dataset with uid ${dataset.uid} already exists for game $gameId, skipping")
} else {
game.datasets.add(dataset)
+ Napier.i("Loaded dataset for game $gameId")
return dataset
}
} ?: Napier.e("Failed to parse dataset for game $gameId, skipping")
return null
}🤖 Prompt for AI Agents
In
`@composeApp/src/commonMain/kotlin/de/benkralex/partygames/datasets/loadDatasets.kt`
around lines 42 - 52, In parseDataset, the Napier.i("Loaded dataset...") is
emitted before checking duplicates; change the flow so you only log the success
message after confirming and adding the dataset to game.datasets (i.e., move the
info log into the else block where game.datasets.add(dataset) and return dataset
occur), and keep or refine the Napier.w warning that references dataset.uid when
the dataset already exists; keep the Napier.e log for the parse failure branch
unchanged.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @Benkralex. * #2 (comment) The following files were modified: * `composeApp/src/commonMain/kotlin/de/benkralex/partygames/app/App.kt` * `composeApp/src/commonMain/kotlin/de/benkralex/partygames/app/GamesRegister.kt` * `composeApp/src/commonMain/kotlin/de/benkralex/partygames/datasets/loadDatasets.kt` * `composeApp/src/commonMain/kotlin/de/benkralex/partygames/gameSelectionPage/presentation/GameSelectionList.kt` * `composeApp/src/commonMain/kotlin/de/benkralex/partygames/gameSelectionPage/presentation/GameSelectionPage.kt` * `composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/common/domain/Game.kt` * `composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/common/presentation/GameSelectionCard.kt` * `composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/common/presentation/PlayGamePage.kt` * `composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/common/presentation/SetupGamePage.kt` * `composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/findLiar/domain/FindLiar.kt` * `composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/impostor/domain/Impostor.kt` * `composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/truthOrDare/domain/TruthOrDare.kt`
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/impostor/domain/Impostor.kt`:
- Around line 40-47: The Impostor setup can allow impostorCount > players.size
which causes the unique-selection while loop in
ImpostorPlayViewModel.initNewRound to never terminate; update createGame (the
code path used by setupGame that constructs ImpostorSettings) to validate and
clamp or reject impostorCount against players.size—e.g., compute safeCount =
min(impostorCount, players.size) or throw an IllegalArgumentException when
impostorCount > players.size—and populate ImpostorSettings.players/impostorCount
with that safe value so the selection loop in initNewRound always terminates.
🧹 Nitpick comments (5)
composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/justOne/domain/JustOne.kt (1)
36-47: Consider renaming the function to avoid shadowing the property.Having both a property and a method named
parseDataworks but can be confusing when reading the code. A clearer approach would be to rename the function.♻️ Suggested rename
- override val parseData: (JsonObject) -> JustOneDataset? = ::parseData + override val parseData: (JsonObject) -> JustOneDataset? = ::parseDataImpl - fun parseData(jsonObject: JsonObject): JustOneDataset? { + private fun parseDataImpl(jsonObject: JsonObject): JustOneDataset? {Alternatively, you could inline the implementation using a lambda:
override val parseData: (JsonObject) -> JustOneDataset? = { jsonObject -> try { json.decodeFromJsonElement<JustOneDataset>(jsonObject) } catch (e: Exception) { Napier.e("Error while decoding Just one dataset", e) null } }composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/findLiar/domain/FindLiar.kt (1)
51-62: Consider extracting duplicated language-matching logic.The language compatibility check is repeated for both
liarQuestionandmainQuestion. Extracting this to a helper would improve readability and reduce duplication.♻️ Suggested helper extraction
private fun TranslatableString.hasLanguageSupport(languages: List<String>): Boolean { val baseLangs = languages.map { it.split("_")[0] } return translations.keys.any { lang -> lang in languages || lang.split("_")[0] in languages || lang in baseLangs } }Then simplify the filter:
- topics = activeDatasets.flatMap { it.questionPairs }.filter { q -> - val languages = de.benkralex.partygames.settingsPage.data.settings.value.languages - q.liarQuestion.translations.keys.any { lang -> - lang in languages - || lang.split("_")[0] in languages - || lang in languages.map { it.split("_")[0] } - } && q.mainQuestion.translations.keys.any { lang -> - lang in languages - || lang.split("_")[0] in languages - || lang in languages.map { it.split("_")[0] } - } - }.map { it.topic }.toSet().toList(), + topics = run { + val languages = de.benkralex.partygames.settingsPage.data.settings.value.languages + activeDatasets.flatMap { it.questionPairs } + .filter { q -> + q.liarQuestion.hasLanguageSupport(languages) && + q.mainQuestion.hasLanguageSupport(languages) + } + .map { it.topic } + .toSet() + .toList() + },composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/truthOrDare/domain/TruthOrDareSettings.kt (1)
3-7: Consider serialization + age-range invariant (if settings are persisted).
If these settings are saved/loaded (e.g., via kotlinx.serialization), annotate with@Serializable. Also consider enforcingageMin <= ageMaxto prevent invalid ranges at the source.♻️ Proposed refactor
+import kotlinx.serialization.Serializable + +@Serializable data class TruthOrDareSettings( val topics: List<String>, val ageMin: Int?, val ageMax: Int?, -) +) { + init { + require(ageMin == null || ageMax == null || ageMin <= ageMax) { + "ageMin must be <= ageMax" + } + } +}composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/truthOrDare/domain/TruthOrDare.kt (1)
43-46: Optional: defensively copy topics to avoid external mutation.
If callers pass a mutable list, later mutations could leak into game state. Consider copying on assignment.♻️ Suggested tweak
override fun createGame( settings: TruthOrDareSettings ) { - this.settings = settings + this.settings = settings.copy(topics = settings.topics.toList()) }composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/impostor/domain/ImpostorSettings.kt (1)
5-9: Tightenhint’s type to preserve the type-safety gains.
Any?reintroduces unchecked casts in a PR focused on stronger typing. Ifhintis textual, considerTranslatableString?; otherwise model it as a sealed type to keep the API explicit.♻️ Example (if hint is localized text)
data class ImpostorSettings( val players: List<String>, val impostorCount: Int, val topics: List<TranslatableString>, - val hint: Any? = null, + val hint: TranslatableString? = null, )
| setupGame = { players, impostorCount, topics -> | ||
| createGame( | ||
| settings = mapOf( | ||
| "players" to players, | ||
| "impostorCount" to impostorCount, | ||
| "topics" to topics, | ||
| settings = ImpostorSettings( | ||
| players = players, | ||
| impostorCount = impostorCount, | ||
| topics = topics, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Validate impostorCount against players to prevent a potential infinite loop.
ImpostorPlayViewModel.initNewRound() (composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/impostor/presentation/ImpostorPlayViewModel.kt, lines 13-95 in the snippet) selects unique impostors in a while loop. If impostorCount > players.size, the loop never terminates. Enforce this at createGame (or clamp in setup).
✅ Proposed guard in createGame
override fun createGame(
settings: ImpostorSettings
) {
+ require(settings.impostorCount in 1..settings.players.size) {
+ "impostorCount must be between 1 and players.size"
+ }
this.settings = settings
activeGame = getKeyByGame(this)
}Also applies to: 74-77
🤖 Prompt for AI Agents
In
`@composeApp/src/commonMain/kotlin/de/benkralex/partygames/games/impostor/domain/Impostor.kt`
around lines 40 - 47, The Impostor setup can allow impostorCount > players.size
which causes the unique-selection while loop in
ImpostorPlayViewModel.initNewRound to never terminate; update createGame (the
code path used by setupGame that constructs ImpostorSettings) to validate and
clamp or reject impostorCount against players.size—e.g., compute safeCount =
min(impostorCount, players.size) or throw an IllegalArgumentException when
impostorCount > players.size—and populate ImpostorSettings.players/impostorCount
with that safe value so the selection loop in initNewRound always terminates.
2 participants
Reduces strange casts & adds more type safety. #1 will require adjustments if this is merged.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.