Refactor stall upgrade logic into a centralized manager#113
Conversation
- Created StallUpgradeManager to centralize upgrade calculations. - Refactored MainViewModel.applyUpgrade to use the new manager. - Refactored StallDefinition.getUpgradeBenefit to use the new manager. - Unified scaling rules and milestone boosts according to STALL_STATS.md. - Improved rounding accuracy using roundToInt and roundToLong. Co-authored-by: candour <4670475+candour@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughRefactors stall upgrade behavior by extracting cost calculation, available-stat selection, per-stat application (including milestone and legendary naming logic), and benefit-string generation from MainViewModel and Registry into a new StallUpgradeManager utility. ChangesStall upgrade centralization
Sequence Diagram(s)sequenceDiagram
participant VM as MainViewModel
participant Mgr as StallUpgradeManager
participant Reg as Registry / UI
VM->>Mgr: calculateUpgradeCost(stall, isSpecific, hasFreeUpgrade)
VM->>Mgr: if not specific → getAvailableUpgradeStats(stall).random()
VM->>Mgr: applyUpgrade(stall, statName, upgradeCost, isSpecific)
Mgr-->>VM: updated Stall
VM->>VM: adjust gold / freeSpecificUpgrades / disabledWaves
Reg->>Mgr: getBenefitString(category, level, baseStall)
Mgr-->>Reg: formatted benefit string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
Build Successful! 🚀Note: This link will be removed when the PR is closed. |
![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
🧹 Nitpick comments (1)
app/src/main/java/com/messark/hawker/utils/StallUpgradeManager.kt (1)
212-212: 💤 Low valueUse explicit locale for consistent decimal formatting.
String.formatuses the device's default locale, which can produce inconsistent decimal separators (e.g., "0,5" vs "0.5") depending on user settings.♻️ Proposed fix
- "+${String.format("%.1f", currentRange - baseStall.range)}" + "+${String.format(java.util.Locale.US, "%.1f", currentRange - baseStall.range)}"- "+${String.format("%.1f", currentRadius - baseStall.aoeRadius)}" + "+${String.format(java.util.Locale.US, "%.1f", currentRadius - baseStall.aoeRadius)}"Also applies to: 220-220
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c7ea660f-b8db-4a1a-b18a-b04323acc135
📒 Files selected for processing (3)
app/src/main/java/com/messark/hawker/MainViewModel.ktapp/src/main/java/com/messark/hawker/registry/Registry.ktapp/src/main/java/com/messark/hawker/utils/StallUpgradeManager.kt
- Centralized upgrade logic in StallUpgradeManager.kt. - Unified scaling rules and milestone boosts. - Fixed inconsistent Rate benefit calculation in getBenefitString. - Added missing imports and improved rounding accuracy. Co-authored-by: candour <4670475+candour@users.noreply.github.com>
Build Successful! 🚀Note: This link will be removed when the PR is closed. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/com/messark/hawker/utils/StallUpgradeManager.kt (1)
214-214: 💤 Low valueUse explicit locale for
String.formatto ensure consistent decimal formatting.The implicit default locale may produce unexpected separators (e.g.,
,instead of.) in some regions, causing UI display inconsistencies.🔧 Proposed fix
+import java.util.Locale ... - "+${String.format("%.1f", currentRange - baseStall.range)}" + "+${String.format(Locale.US, "%.1f", currentRange - baseStall.range)}" ... - "+${String.format("%.1f", currentRadius - baseStall.aoeRadius)}" + "+${String.format(Locale.US, "%.1f", currentRadius - baseStall.aoeRadius)}"Also applies to: 222-222
app/src/main/java/com/messark/hawker/MainViewModel.kt (1)
10-10: 💤 Low valueRedundant import.
StallUpgradeManageris already covered by the wildcard import on line 9 (import com.messark.hawker.utils.*).🧹 Proposed fix
import com.messark.hawker.utils.* -import com.messark.hawker.utils.StallUpgradeManager
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 28ba2823-3d91-401b-8cf1-1a4c0dd9ffbc
📒 Files selected for processing (2)
app/src/main/java/com/messark/hawker/MainViewModel.ktapp/src/main/java/com/messark/hawker/utils/StallUpgradeManager.kt
1 participant
This change refactors the stall upgrade logic, which was previously duplicated and inconsistent between MainViewModel.kt and Registry.kt, into a new centralized StallUpgradeManager.kt. This manager handles stat scaling, milestone boosts, cost calculations, and legendary naming, ensuring consistency between gameplay logic and UI descriptions. Scaling rules have been unified according to STALL_STATS.md.
PR created automatically by Jules for task 13391773198545734455 started by @candour
Summary by CodeRabbit
Refactor
User Impact