Different approach to refactor - #69
Conversation
This commit finalizes the refactoring effort by: - Extracting stall and enemy configuration data into and . - Moving associated logic into and . - Centralizing global game parameters and type-specific constants into , , , and enum. - Removing the obsolete file and unused imports. - Updating and logic handlers to utilize the new structures and constants. These changes improve code organization, readability, and maintainability by separating data from logic and making game balance parameters more accessible for tuning.
This commit finalizes the refactoring effort by: - Extracting stall and enemy configuration data into and . - Moving associated logic into and . - Centralizing global game parameters and type-specific constants into , , , and enum. - Removing the obsolete file and unused imports. - Updating and logic handlers to utilize the new structures and constants. These changes improve code organization, readability, and maintainability by separating data from logic and making game balance parameters more accessible for tuning.
📝 WalkthroughWalkthroughThis PR refactors the game architecture from a registry-based pattern to a data-driven pattern. It removes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (5)
app/src/main/java/com/messark/hawkerrush/ui/constants/EnemyData.kt (1)
3-4: Unused imports.
ColorandIntRectare imported but not used in this file. They may have been carried over from another file during refactoring.♻️ Remove unused imports
package com.messark.hawkerrush.ui.constants -import androidx.compose.ui.graphics.Color -import androidx.compose.ui.unit.IntRect import com.messark.hawkerrush.model.*app/src/main/java/com/messark/hawkerrush/ui/constants/StallData.kt (1)
68-68: Type consistency: UseLongliteral forfireRateMs.For consistency with other
Longvalues in the file (e.g., line 53:1000L), consider using1500Linstead of1500. Same applies to lines 89, 103, and 119.♻️ Use Long literals consistently
- fireRateMs = 1500, + fireRateMs = 1500L,app/src/main/java/com/messark/hawkerrush/logic/EnemyBehaviorHandler.kt (2)
57-67: Use explicit locale forString.formatto ensure consistent formatting.
String.format("%.1f", ...)uses the device's default locale, which may produce inconsistent decimal separators (e.g., "0.5" vs "0,5") across regions.♻️ Use explicit Locale
+import java.util.Locale + // In getUpgradeBenefit function: - "+${String.format("%.1f", currentRange - stallConfig.range)}" + "+${String.format(Locale.US, "%.1f", currentRange - stallConfig.range)}" // ... - "+${String.format("%.1f", currentRadius - stallConfig.aoeRadius)}" + "+${String.format(Locale.US, "%.1f", currentRadius - stallConfig.aoeRadius)}"
277-283: UnusedenemyConfigparameter.The
enemyConfigparameter is never used in this function. SinceenemyTypeis passed separately, consider removing the unused parameter or derivingenemyTypefromenemyConfig.type.♻️ Remove unused parameter
- fun getPuddleSlowMultiplier(enemyConfig: EnemyConfig, enemyType: EnemyType): Float { + fun getPuddleSlowMultiplier(enemyType: EnemyType): Float { return when (enemyType) { EnemyType.DELIVERY_RIDER -> 0.2f EnemyType.AUNTIE -> 0.8f else -> 0.6f } }Then update the caller in
MainViewModel.ktline 428:- speedMultiplier = EnemyBehaviorHandler.getPuddleSlowMultiplier(enemyConfig, enemy.type) + speedMultiplier = EnemyBehaviorHandler.getPuddleSlowMultiplier(enemy.type)app/src/main/java/com/messark/hawkerrush/logic/StallActionHandler.kt (1)
6-7: Unused import:EnemyData.
EnemyDatais imported but not used inStallActionHandler. This is dead code from the refactoring.♻️ Remove unused import
import com.messark.hawkerrush.ui.constants.StallData -import com.messark.hawkerrush.ui.constants.EnemyData import java.util.*
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5dda3dbb-78ed-4786-9c83-9c9e09281cfb
📒 Files selected for processing (6)
app/src/main/java/com/messark/hawkerrush/MainViewModel.ktapp/src/main/java/com/messark/hawkerrush/logic/EnemyBehaviorHandler.ktapp/src/main/java/com/messark/hawkerrush/logic/StallActionHandler.ktapp/src/main/java/com/messark/hawkerrush/registry/Registry.ktapp/src/main/java/com/messark/hawkerrush/ui/constants/EnemyData.ktapp/src/main/java/com/messark/hawkerrush/ui/constants/StallData.kt
💤 Files with no reviewable changes (1)
- app/src/main/java/com/messark/hawkerrush/registry/Registry.kt
| package com.messark.hawkerrush.logic | ||
|
|
||
| import androidx.compose.ui.graphics.Color | ||
| import com.messark.hawkerrush.model.* | ||
| import com.messark.hawkerrush.registry.FireResult | ||
| import com.messark.hawkerrush.ui.constants.StallData | ||
| import com.messark.hawkerrush.ui.constants.EnemyData | ||
| import com.messark.hawkerrush.ui.constants.EnemyConstants | ||
| import com.messark.hawkerrush.ui.constants.GameConstants // Import GameConstants | ||
| import com.messark.hawkerrush.ui.constants.StallConstants | ||
| import com.messark.hawkerrush.ui.constants.StallUpgradeCategory | ||
| import java.util.* |
There was a problem hiding this comment.
File contains both handlers but should only have EnemyBehaviorHandler.
This file (EnemyBehaviorHandler.kt) contains both StallActionHandler and EnemyBehaviorHandler. Based on the file list, there's a separate StallActionHandler.kt file. Having duplicate definitions will cause compilation errors due to duplicate class declarations.
Additionally, line 9 imports GameConstants from ui.constants, but GameConstants is defined in MainViewModel.kt (line 21-32). This import path is incorrect and will cause a compilation error.
🐛 Remove StallActionHandler and fix import
Either:
- Remove
StallActionHandlerfrom this file (keep onlyEnemyBehaviorHandler), OR - Move
GameConstantstoui.constantspackage
package com.messark.hawkerrush.logic
import androidx.compose.ui.graphics.Color
import com.messark.hawkerrush.model.*
import com.messark.hawkerrush.registry.FireResult
import com.messark.hawkerrush.ui.constants.StallData
import com.messark.hawkerrush.ui.constants.EnemyData
import com.messark.hawkerrush.ui.constants.EnemyConstants
-import com.messark.hawkerrush.ui.constants.GameConstants // Import GameConstants
+import com.messark.hawkerrush.GameConstants // GameConstants is in MainViewModel.kt
import com.messark.hawkerrush.ui.constants.StallConstants
import com.messark.hawkerrush.ui.constants.StallUpgradeCategory
import java.util.*📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| package com.messark.hawkerrush.logic | |
| import androidx.compose.ui.graphics.Color | |
| import com.messark.hawkerrush.model.* | |
| import com.messark.hawkerrush.registry.FireResult | |
| import com.messark.hawkerrush.ui.constants.StallData | |
| import com.messark.hawkerrush.ui.constants.EnemyData | |
| import com.messark.hawkerrush.ui.constants.EnemyConstants | |
| import com.messark.hawkerrush.ui.constants.GameConstants // Import GameConstants | |
| import com.messark.hawkerrush.ui.constants.StallConstants | |
| import com.messark.hawkerrush.ui.constants.StallUpgradeCategory | |
| import java.util.* | |
| package com.messark.hawkerrush.logic | |
| import androidx.compose.ui.graphics.Color | |
| import com.messark.hawkerrush.model.* | |
| import com.messark.hawkerrush.registry.FireResult | |
| import com.messark.hawkerrush.ui.constants.StallData | |
| import com.messark.hawkerrush.ui.constants.EnemyData | |
| import com.messark.hawkerrush.ui.constants.EnemyConstants | |
| import com.messark.hawkerrush.GameConstants | |
| import com.messark.hawkerrush.ui.constants.StallConstants | |
| import com.messark.hawkerrush.ui.constants.StallUpgradeCategory | |
| import java.util.* |
| private fun getUpgradeDamageIncrease(baseDamage: Int): Int { | ||
| return (baseDamage * 0.3f).toInt() + 2 | ||
| } |
There was a problem hiding this comment.
Critical: getUpgradeDamageIncrease is private but called from MainViewModel.
MainViewModel.kt (lines 816, 881) calls StallActionHandler.getUpgradeDamageIncrease(...), but this function is declared private. This will cause a compilation error.
🐛 Change visibility to internal or public
- private fun getUpgradeDamageIncrease(baseDamage: Int): Int {
+ fun getUpgradeDamageIncrease(baseDamage: Int): Int {
return (baseDamage * 0.3f).toInt() + 2
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun getUpgradeDamageIncrease(baseDamage: Int): Int { | |
| return (baseDamage * 0.3f).toInt() + 2 | |
| } | |
| fun getUpgradeDamageIncrease(baseDamage: Int): Int { | |
| return (baseDamage * 0.3f).toInt() + 2 | |
| } |
| /** | ||
| * Handles actions and logic related to enemies. | ||
| */ | ||
| object EnemyBehaviorHandler { | ||
|
|
||
| /** | ||
| * Updates enemy behavior, including special actions like stopping for tourists. | ||
| * This logic was previously in EnemyDefinition.updateSpecialBehavior(). | ||
| */ | ||
| fun updateSpecialBehavior(enemyConfig: EnemyConfig, enemy: Enemy, currentTimeMs: Long): Enemy { | ||
| if (enemyConfig.type == EnemyType.TOURIST) { | ||
| var isStopped = enemy.isStopped | ||
| var stopDurationMs = enemy.stopDurationMs | ||
| var lastStopMs = enemy.lastStopMs | ||
|
|
||
| if (isStopped) { | ||
| stopDurationMs -= 32 // Game tick duration | ||
| if (stopDurationMs <= 0) { | ||
| isStopped = false | ||
| lastStopMs = currentTimeMs | ||
| } | ||
| } else if (currentTimeMs - lastStopMs > 8000) { // Cooldown before stopping again | ||
| isStopped = true | ||
| stopDurationMs = 2000L // Duration of the stop | ||
| } | ||
| return enemy.copy(isStopped = isStopped, stopDurationMs = stopDurationMs, lastStopMs = lastStopMs) | ||
| } | ||
| return enemy | ||
| } | ||
|
|
||
| /** | ||
| * Calculates the slow multiplier for enemies when they are in a puddle. | ||
| * This logic was previously in EnemyDefinition.getPuddleSlowMultiplier(). | ||
| */ | ||
| fun getPuddleSlowMultiplier(enemyConfig: EnemyConfig, enemyType: EnemyType): Float { | ||
| return when (enemyType) { | ||
| EnemyType.DELIVERY_RIDER -> 0.2f | ||
| EnemyType.AUNTIE -> 0.8f | ||
| else -> 0.6f | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Calculates the current HP of an enemy based on the wave number. | ||
| * This logic was previously in EnemyDefinition.getHp(). | ||
| */ | ||
| fun getEnemyHpForWave(enemyConfig: EnemyConfig, wave: Int): Int { | ||
| return (enemyConfig.baseHp * Math.pow(1.1, (wave - 1).toDouble())).toInt() | ||
| } | ||
|
|
||
| /** | ||
| * Creates an Enemy instance from its configuration. | ||
| * This logic was previously in EnemyDefinition.toEnemy(). | ||
| */ | ||
| fun createEnemyInstance( | ||
| enemyConfig: EnemyConfig, | ||
| id: String = UUID.randomUUID().toString(), | ||
| wave: Int, | ||
| position: PreciseAxialCoordinate, | ||
| path: List<AxialCoordinate>, | ||
| isFacingLeft: Boolean | ||
| ): Enemy { | ||
| val hp = getEnemyHpForWave(enemyConfig, wave) | ||
| return Enemy( | ||
| id = id, | ||
| type = enemyConfig.type, | ||
| health = hp, | ||
| maxHealth = hp, | ||
| position = position, | ||
| baseSpeed = enemyConfig.baseSpeed, | ||
| currentSpeed = enemyConfig.baseSpeed, | ||
| path = path, | ||
| currentPathIndex = 0, | ||
| reward = enemyConfig.reward, | ||
| isFacingLeft = isFacingLeft | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Duplicate EnemyBehaviorHandler definition.
EnemyBehaviorHandler is defined in both StallActionHandler.kt (lines 244-321) and EnemyBehaviorHandler.kt. This will cause a duplicate class compilation error. Additionally, this version uses hardcoded values (lines 260, 265-267, 291) while EnemyBehaviorHandler.kt uses constants.
🐛 Remove the duplicate EnemyBehaviorHandler from this file
Remove lines 244-321 entirely. EnemyBehaviorHandler should only exist in EnemyBehaviorHandler.kt.
}
}
-
-/**
- * Handles actions and logic related to enemies.
- */
-object EnemyBehaviorHandler {
- // ... entire object removed ...
-}| val dq = targetPos.q - proj.position.q | ||
| val dr = targetPos.r - proj.position.r | ||
| val dr = target.position.r - proj.position.r | ||
| val dist = axialDistance(proj.position, targetPos) |
There was a problem hiding this comment.
Critical: Undefined variable target causing compilation error.
Line 574 references target which is not defined in this scope. Based on the context and the pattern on line 573 (targetPos.q), this should be targetPos.r.
🐛 Fix the undefined variable reference
val dq = targetPos.q - proj.position.q
- val dr = target.position.r - proj.position.r
+ val dr = targetPos.r - proj.position.r
val dist = axialDistance(proj.position, targetPos)| // Use StallActionHandler to create stall instance | ||
| val stallConfig = StallData.configs[stallToPlace.stallType] ?: return@update // Should not happen | ||
| newHexes[coord] = tile.copy(stall = StallActionHandler.createStallInstance(stallConfig)) |
There was a problem hiding this comment.
Critical: Invalid return@update label reference.
Line 735-736 uses return@update but this code is not inside the _gameState.update lambda (which starts at line 738). This will cause a compilation error.
🐛 Fix the return statement
// Use StallActionHandler to create stall instance
- val stallConfig = StallData.configs[stallToPlace.stallType] ?: return@update // Should not happen
+ val stallConfig = StallData.configs[stallToPlace.stallType] ?: return
newHexes[coord] = tile.copy(stall = StallActionHandler.createStallInstance(stallConfig))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use StallActionHandler to create stall instance | |
| val stallConfig = StallData.configs[stallToPlace.stallType] ?: return@update // Should not happen | |
| newHexes[coord] = tile.copy(stall = StallActionHandler.createStallInstance(stallConfig)) | |
| // Use StallActionHandler to create stall instance | |
| val stallConfig = StallData.configs[stallToPlace.stallType] ?: return | |
| newHexes[coord] = tile.copy(stall = StallActionHandler.createStallInstance(stallConfig)) |
| reward = 100, | ||
| spriteRow = 3 | ||
| ) | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Critical: Duplicate code causing syntax error.
Lines 75-80 are an exact duplicate of lines 69-74. This will cause a compilation error due to duplicate closing braces and repeated map entries.
🐛 Remove the duplicate lines
)
)
}
- reward = 100,
- spriteRow = 3
- )
- )
-}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| reward = 100, | |
| spriteRow = 3 | |
| ) | |
| ) | |
| } | |
| reward = 100, | |
| spriteRow = 3 | |
| ) | |
| ) | |
| } | |
2 participants
Refactor: Decouple stall/enemy data and logic, centralize constants
This commit finalizes the refactoring effort by:
StallData.ktandEnemyData.kt.StallActionHandler.ktandEnemyBehaviorHandler.kt.GameConstants,EnemyConstants,StallConstants, andStallUpgradeCategoryenum.Registry.ktfile and unused imports.MainViewModel.ktand logic handlers to utilize the new structures and constants.These changes improve code organization, readability, and maintainability by separating data from logic and making game balance parameters more accessible for tuning.
Summary by CodeRabbit