Skip to content

Refactor StallUpgradeManager architecture#160

Closed
candour wants to merge 2 commits into
mainfrom
refactor-upgrade-manager-4147648448634557213
Closed

Refactor StallUpgradeManager architecture#160
candour wants to merge 2 commits into
mainfrom
refactor-upgrade-manager-4147648448634557213

Conversation

@candour

@candour candour commented May 15, 2026

Copy link
Copy Markdown
Owner

This PR refactors StallUpgradeManager.kt to replace a complex, branching fold loop with a declarative strategy-based architecture.

Key improvements:

  • Scalability: New stats or scaling rules can be added by implementing the StatScaler interface without modifying the core calculation engine.
  • Maintainability: Scaling logic for specific stats (like Rate floors or Chicken Rice damage) is now encapsulated in dedicated classes.
  • Robustness: applyUpgrade now includes a clean normalization pass to ensure that aliased stats (e.g., "Grab Rate" vs "Rate") stay in sync, preventing legacy data bugs.

Verified with the following test suites:

  • StallStatsTest
  • MilestoneBoostTest
  • AliasNormalizationTest
  • LegendaryNamingTest

PR created automatically by Jules for task 4147648448634557213 started by @candour

Summary by CodeRabbit

  • Refactor
    • Improved internal code organization and maintainability of upgrade system logic, with no impact to user-facing functionality.

Review Change Stack

- Introduced StatScaler interface and specialized implementations for each stat type.
- Replaced monolithic branching logic in calculateValue with a strategy pattern delegation.
- Simplified applyUpgrade to handle stat normalization and level synchronization more efficiently.
- Improved code maintainability and readability by separating scaling rules from core logic.
- Updated fixes.md with refactoring record REF-010.

Co-authored-by: candour <4670475+candour@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai

coderabbitai Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@candour has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 55 minutes and 19 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e5ee581f-72ca-4f2a-9d6d-83e456ccb2f6

📥 Commits

Reviewing files that changed from the base of the PR and between 6a811ff and 6aaf249.

📒 Files selected for processing (1)
  • app/src/main/java/com/messark/hawker/utils/StallUpgradeManager.kt
📝 Walkthrough

Walkthrough

StallUpgradeManager refactors stat-upgrade computation from iterative scaling logic into a declarative StatScaler abstraction. Seven concrete scaler implementations compute stat values, calculateValue delegates to the scaler map, applyUpgrade normalizes levels only across available stats and updates legendary naming rules, and a changelog entry documents the refactor.

Changes

Upgrade Manager Refactor

Layer / File(s) Summary
StatScaler abstraction and scaler implementations
app/src/main/java/com/messark/hawker/utils/StallUpgradeManager.kt
Introduces StatScaler interface with seven concrete implementations (Damage, Range, Rate, Radius, Duration, Effect, Boost) and creates a scalers map keyed by canonical stat name for centralized upgrade-value computation.
Value calculation refactoring
app/src/main/java/com/messark/hawker/utils/StallUpgradeManager.kt
Updates calculateValue to select the appropriate scaler by canonical stat name and compute per-level folded values via scaler.calculate(...), with results memoized through valueCache.
Upgrade application and legendary naming
app/src/main/java/com/messark/hawker/utils/StallUpgradeManager.kt
Refactors applyUpgrade to normalize levels only across available upgrade stat keys, manage legendary naming by directly mutating stall.namingCategories with prefix/suffix rules at level 10, and recompute stall stats using calculateValue with a getLevel helper that resolves canonical levels from mutable upgrades.
Refactor documentation
fixes.md
Adds REF-010 changelog entry documenting the StatScaler architecture introduction, removal of iterative when-based scaling, and upgrade-logic maintainability improvements.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • candour/towerpower#119: The main PR's applyUpgrade bases level normalization on getAvailableUpgradeStats(stall) keys, and PR #119 changes that function to return emptyList() for StallType.ATM with a guard, so they directly interact at upgrade-selection logic.
  • candour/towerpower#128: Both PRs refactor StallUpgradeManager.kt's stat/upgrade computation with canonicalized stat-level handling and overhaul calculateValue/upgrade application/legendary naming at the same code paths.
  • candour/towerpower#116: Both PRs refactor StallUpgradeManager.kt upgrade/stat computation and applyUpgrade logic to recompute values from canonical stats, unifying scaling/normalization and adjusting legendary/benefit calculations.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Refactor StallUpgradeManager architecture' directly and clearly summarizes the main change in the pull request, which is a comprehensive refactoring of the StallUpgradeManager.kt architecture from a complex branching approach to a declarative strategy-based design.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor-upgrade-manager-4147648448634557213

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown

Build Successful! 🚀

Download APK

Note: This link will be removed when the PR is closed.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ada3784e-13cf-46b5-bbe7-fbc38d5a056f

📥 Commits

Reviewing files that changed from the base of the PR and between 132e93b and 6a811ff.

📒 Files selected for processing (2)
  • app/src/main/java/com/messark/hawker/utils/StallUpgradeManager.kt
  • fixes.md

Comment thread app/src/main/java/com/messark/hawker/utils/StallUpgradeManager.kt
@candour

candour commented May 16, 2026

Copy link
Copy Markdown
Owner Author

Normalize the legacy Damage alias for BAK_KUT_TEH before reading levels.

getAvailableUpgradeStats() only returns "Boost" for StallType.BAK_KUT_TEH, so this pass never copies an existing legacy "Damage" level into "Boost". newLevel and the later getLevel() helper then read only "Boost", which resets persisted upgrade progress on the next apply.

@google-labs-jules

Copy link
Copy Markdown
Contributor

Normalize the legacy Damage alias for BAK_KUT_TEH before reading levels.

getAvailableUpgradeStats() only returns "Boost" for StallType.BAK_KUT_TEH, so this pass never copies an existing legacy "Damage" level into "Boost". newLevel and the later getLevel() helper then read only "Boost", which resets persisted upgrade progress on the next apply.

Good catch. I've updated the normalization pass to iterate over both the existing upgrade keys and the available stats. This ensures that legacy aliases (like 'Damage' for Bak Kut Teh) are correctly mapped to their canonical forms ('Boost') before the level is read, preventing progress resets.

- Updated normalization pass in applyUpgrade to iterate over all existing upgrade keys.
- This ensures that legacy aliases (e.g., 'Damage' for BKT) are correctly mapped to canonical names ('Boost') even if not returned by getAvailableUpgradeStats.
- Prevents upgrade progress reset when applying new upgrades to legacy stalls.
- Verified with unit tests.

Co-authored-by: candour <4670475+candour@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

Build Successful! 🚀

Download APK

Note: This link will be removed when the PR is closed.

@candour
candour enabled auto-merge (squash) May 16, 2026 01:25
@candour

candour commented May 16, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai resolve

@coderabbitai

coderabbitai Bot commented May 16, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Comments resolved and changes approved.

@candour
candour disabled auto-merge May 17, 2026 07:10
@candour

candour commented May 17, 2026

Copy link
Copy Markdown
Owner Author

@jules please rebase and fix merge conflicts

@candour candour closed this May 17, 2026
@candour
candour deleted the refactor-upgrade-manager-4147648448634557213 branch May 17, 2026 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant