Small refactor: nearbyUnits readonly UnitType[]#3236
Conversation
…(or others in the future) is needed anymore when sending it as argument.
WalkthroughThe pull request updates the Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/game/Game.ts (1)
785-799: Optional: makeanyUnitNearbyandnearbyUnitsconsistent with a single-type shorthand.
anyUnitNearby(line 788) accepts onlyreadonly UnitType[], butnearbyUnits(line 796) acceptsUnitType | readonly UnitType[]— so a caller with a singleUnitTypemust wrap it in an array foranyUnitNearbybut not fornearbyUnits. This asymmetry pre-dates this PR. If you want to eliminate the inconsistency, align both toUnitType | readonly UnitType[]:♻️ Align anyUnitNearby with nearbyUnits
anyUnitNearby( tile: TileRef, searchRange: number, - types: readonly UnitType[], + types: UnitType | readonly UnitType[], predicate: (unit: Unit) => boolean, playerId?: PlayerID, includeUnderConstruction?: boolean, ): boolean;The same change would propagate to
GameImpl.ts,GameView.ts, andUnitGrid.tsfor consistency — but that is entirely separate from this PR's scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/Game.ts` around lines 785 - 799, The two APIs are inconsistent: change the anyUnitNearby signature to accept UnitType | readonly UnitType[] (matching nearbyUnits) so callers can pass a single UnitType without wrapping; update the declaration in Game.ts (anyUnitNearby) and then update all implementations and overrides (e.g., GameImpl.anyUnitNearby, GameView.anyUnitNearby, UnitGrid.anyUnitNearby) to accept the same union type, adjust internal handling to treat a single UnitType as a one-element array where needed, and update any call sites that assumed the old type accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/game/Game.ts`:
- Around line 785-799: The two APIs are inconsistent: change the anyUnitNearby
signature to accept UnitType | readonly UnitType[] (matching nearbyUnits) so
callers can pass a single UnitType without wrapping; update the declaration in
Game.ts (anyUnitNearby) and then update all implementations and overrides (e.g.,
GameImpl.anyUnitNearby, GameView.anyUnitNearby, UnitGrid.anyUnitNearby) to
accept the same union type, adjust internal handling to treat a single UnitType
as a one-element array where needed, and update any call sites that assumed the
old type accordingly.
|
Lgtm |
## Description: PR 5/x in effort to break up PR #3220. Follows on already merged #3236. Please see if these can be merged for v30. **NationStructureBehavior**: - maybeSpawnStructure: cache this.game to be used twice. - maybeSpawnStructure: instead of hardcoded ruling out Defense Post for upgrade check, check dynamically if type is upgradable. That way if defense posts ever do become upgradable, we don't run into a bug right away. - maybeUpgradeStructure: removed canUpgradeUnit check. Since it already checked this right before in findBestStructureToUpgrade, so only upgradable units are returned. And canUpgradeUnit is also checked right after in UpgradeStructureExecution. So we're going from 3 times to 2 times canUpgradeUnit, small perf win too. - findBestStructureToUpgrade: cache this.game to be used thrice. - shouldBuildStructure: cache this.game.config() to be used twice. - getTotalStructureDensity: this.player.units can handle an array of unit types to count. Input StructureTypes like this so we don't need a loop and count, and only have to get an array length once. getTotalStructureDensity needs to ignore unit levels so we can't make use of other pre-defined functions in PlayerImpl (which were created to avoid array length calls), but at least this saves a few. ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: tryout33
Description:
PR 4/x in effort to break up PR #3220. Follows on already merged #3235. Precedes #3237.
Please see if these can be merged for v30.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tryout33