Perf/small refactor: NationStructureBehavior

[VariableVince]

Description:

PR 5/x in effort to break up PR #3220. Follows on already merged #3236. Precedes #3238.

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:

• I have added screenshots for all UI updates
• I process any text displayed to the user through translateText() and I've added it to the en.json file
• I have added relevant tests to the test directory
• 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

[coderabbitai]

Walkthrough

Single-file refactoring that caches repeated game.config() lookups into local variables, simplifies upgrade validation logic by delegating checks to execution handlers, and optimizes structure density calculation using direct unit filtering.

Changes

Cohort / File(s)	Summary
Config caching & reference consolidation src/core/execution/nation/NationStructureBehavior.ts	Introduced local gameConfig and game variables to replace repeated this.game.config() calls across shouldBuildStructure, maybeSpawnStructure, and SAM-distance scoring logic, reducing redundant lookups.
Upgrade validation simplification src/core/execution/nation/NationStructureBehavior.ts	Removed explicit canUpgradeUnit check from main upgrade trigger and relaxed validation; added early filtering in findBestStructureToUpgrade via canUpgradeUnit; replaced checks in maybeUpgradeStructure with null-guard logic, delegating validation to UpgradeStructureExecution.
Density calculation refactor src/core/execution/nation/NationStructureBehavior.ts	Replaced manual structure aggregation in getTotalStructureDensity with single .length calculation using this.player.units(...StructureTypes).length for clarity and efficiency.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🔄 Caching whispers, lookups dance,
Local refs spin their swift romance,
Density flows through cleaner streams,
One file shines with optimized dreams! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title refers to real changes in the PR but is vague and generic (using 'some refactor/perf changes'), making it unclear what specific improvements are being made.	Use a more descriptive title that highlights the main improvements, such as 'Cache game config and optimize structure upgrade checks in NationStructureBehavior' or 'Reduce redundant canUpgradeUnit checks and cache config in NationStructureBehavior'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description clearly explains all the changes made to NationStructureBehavior, references related PRs, and provides specific rationale for each modification.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (2)

src/core/execution/nation/NationStructureBehavior.ts (2)

176-177: Optional: align local variable name with the config convention used elsewhere.

shouldBuildStructure names this.game.config() as gameConfig, but handleStructures (line 94) and getSaveUpTarget (line 295) both call the same value config. The name gameConfig is also mildly confusing because the next line calls gameConfig.gameConfig(), making the chain look redundant at first glance.

♻️ Suggested rename

-    const gameConfig = this.game.config();
-    const { difficulty } = gameConfig.gameConfig();
+    const config = this.game.config();
+    const { difficulty } = config.gameConfig();
     const ratios = getStructureRatios(difficulty);
     ...
-      !gameConfig.isUnitDisabled(UnitType.Port)
+      !config.isUnitDisabled(UnitType.Port)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/execution/nation/NationStructureBehavior.ts` around lines 176 - 177,
In shouldBuildStructure replace the local variable named gameConfig with the
same convention used elsewhere (e.g., config) to avoid confusion with the
subsequent gameConfig.gameConfig() call; locate the assignment where
this.game.config() is stored and rename that variable to config (consistent with
handleStructures and getSaveUpTarget) and update the following destructure line
that reads gameConfig.gameConfig() to config.gameConfig() so names are
consistent and clearer.

---

398-418: Optional: hoist samRange and game.config() out of the inner loop.

game.config().samRange(sam.level()) at line 405 is called once per (structure, sam) pair, but it depends only on the SAM's level — not on the structure being scored. Precomputing a samRange for each SAM before the scoring loop removes the repeated game.config() call from the inner body.

♻️ Suggested optimization

+    const cfg = game.config();
+    // Pre-compute per-SAM range (depends only on level, not on structure)
+    const samRanges = new Map<Unit, number>(
+      samLaunchers.map((sam) => [sam, cfg.samRange(sam.level())]),
+    );
+
     for (const structure of upgradable) {
       let score = 0;
       for (const sam of samLaunchers) {
-        const samRange = game.config().samRange(sam.level());
+        const samRange = samRanges.get(sam)!;
         const samRangeSquared = samRange * samRange;
-        const distSquared = game.euclideanDistSquared(
+        const distSquared = cfg.euclideanDistSquared(

Note: euclideanDistSquared belongs to game, not cfg; only the samRange call moves to cfg.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/execution/nation/NationStructureBehavior.ts` around lines 398 - 418,
The scoring loop repeatedly calls game.config().samRange(sam.level()) for each
(structure, sam) pair which is redundant; precompute per-SAM ranges before
iterating structures by mapping samLaunchers to an array of objects like { sam,
range, rangeSquared } using game.config().samRange(sam.level()), then in the
inner loop use the cached rangeSquared and sam (keeping
game.euclideanDistSquared(structure.tile(), sam.tile()) as-is) so
NationStructureBehavior's scoring logic (references: samLaunchers, upgradable,
samRange, sam.level(), euclideanDistSquared, structure.tile(), sam.tile()) uses
the precomputed values and avoids repeated game.config() calls.

🤖 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/execution/nation/NationStructureBehavior.ts`:
- Around line 176-177: In shouldBuildStructure replace the local variable named
gameConfig with the same convention used elsewhere (e.g., config) to avoid
confusion with the subsequent gameConfig.gameConfig() call; locate the
assignment where this.game.config() is stored and rename that variable to config
(consistent with handleStructures and getSaveUpTarget) and update the following
destructure line that reads gameConfig.gameConfig() to config.gameConfig() so
names are consistent and clearer.
- Around line 398-418: The scoring loop repeatedly calls
game.config().samRange(sam.level()) for each (structure, sam) pair which is
redundant; precompute per-SAM ranges before iterating structures by mapping
samLaunchers to an array of objects like { sam, range, rangeSquared } using
game.config().samRange(sam.level()), then in the inner loop use the cached
rangeSquared and sam (keeping game.euclideanDistSquared(structure.tile(),
sam.tile()) as-is) so NationStructureBehavior's scoring logic (references:
samLaunchers, upgradable, samRange, sam.level(), euclideanDistSquared,
structure.tile(), sam.tile()) uses the precomputed values and avoids repeated
game.config() calls.

[FloPinguin]

thank you, makes sense