Whole repo code review#402
Conversation
| internal class BanUseCase( | ||
| private val adminTool: AdminTool, | ||
| ) { | ||
| suspend fun invoke(origin: Source, query: String): Result<Pair<Ban, Source>, BotError> { |
There was a problem hiding this comment.
Pair is not the easiest to read, you can use data class in moments like that to add clarity
| import io.github.sophon.core.featureConfig.model.FeatureInfo | ||
| import io.github.sophon.discord.feat.core.domain.model.BotOutput | ||
|
|
||
| internal class CreateRedirectButtonsUseCase { |
There was a problem hiding this comment.
extract interface to test
| featureList: List<FeatureInfo> | ||
| ): BotOutput.ButtonSet { | ||
| val buttonList = featureList | ||
| .mapNotNull { featureInfo -> |
There was a problem hiding this comment.
What happens if you receive null list only?
| import io.github.sophon.integration.model.AdminResult | ||
| import io.github.sophon.integration.model.Source | ||
|
|
||
| internal class ProcessFeedbackUseCase( |
There was a problem hiding this comment.
same as above
| import kotlin.time.ExperimentalTime | ||
|
|
||
| @OptIn(ExperimentalTime::class) | ||
| internal class AdminDiscordFeature( |
There was a problem hiding this comment.
this is noice actually, you've extracted all business logic outside of the feature. Now this code is really easy for you to maintain and test so GJ
| private suspend fun feedback( | ||
| origin: Source, | ||
| message: String, | ||
| ): Result<BotOutput, BotError> { |
There was a problem hiding this comment.
side question:
That pattern looks as arrow either, have you ever seen it?
| isEphemeral: Boolean = false, | ||
| ): Result<String, BotError> { | ||
| return try { | ||
| val uuid = Uuid.Companion.random() |
There was a problem hiding this comment.
Generally whenever you see any companion object usage like that it means that your code in the current state can't be tested, you can't just mock parameters. In this example you have to mock static. Consider extracting everything that is used within a given class to something constructor parameter related so that it can be mocked/tested easily. The easier it is to test, the easier it is to maintain
skaluzinski1801
left a comment
skaluzinski1801
left a comment
There was a problem hiding this comment.
Nitpick: Hardcoded strings — no localization support
The entire file uses hardcoded English strings (titles like "ERROR", button labels like "EXAMPLES", "COMMANDS", "HELP", and all description/instruction text). There is no abstraction for translations, making it impossible to localize without rewriting everything.
Consider introducing a translations wrapper:
interface ResourceWrapper {
fun provideTranslation(trans: Translation): String
}
sealed interface Translation {
sealed interface Error : Translation {
data object Title : Error
data class Description(val detail: String) : Error
}
sealed interface Button : Translation {
data object Examples : Button
data object Commands : Button
data object Help : Button
}
// ...
}This way all user-facing strings are resolved through ResourceWrapper, and adding a new locale only requires implementing the interface for that language. The same pattern should be applied across the entire bot module, not just this file.
|
|
||
| val botOutput = BotOutput( | ||
| primaryEmbedBuilder = { | ||
| title = "Join $userName's lobby!" |
There was a problem hiding this comment.
We want those fields to be hardcoded in english?
There was a problem hiding this comment.
Bot stuff is only in English, it's Discord after all.
| } | ||
| } | ||
| } | ||
| .map { } |
There was a problem hiding this comment.
Map because we return EmptyResult.
Hence the map{} is for Unit.
| return mapIndexed { index, move -> | ||
| val query = "$charName ${move.input}" | ||
| BotOutput.EmbedButton( | ||
| label = (index + 1).toString(), |
There was a problem hiding this comment.
index + 1 === magic maths
…ocus Refactor/sorry/move search auto focus
Fix/sorry/dustloop url
Fix/sorry/mizuumi mbtl char icons
First word, then last word, then game icon as fallback
…holder_char_icons refactor (dreamcancel): character icons
…button Refactor/sorry/dustloop feedback button
Refactor/sorry/discord kofi
Refactor/sorry/quiz autoplay
Feat/sorry/home swipe to refresh
2 participants
Full code review comparing the current state of the repository against the initial commit.