Skip to content

Comments

Small perf: find() instead of filter() for retreat#3277

Merged
evanpelle merged 1 commit intomainfrom
retreat-perf
Feb 23, 2026
Merged

Small perf: find() instead of filter() for retreat#3277
evanpelle merged 1 commit intomainfrom
retreat-perf

Conversation

@VariableVince
Copy link
Contributor

Description:

Only need find() instead of filter() in orderRetreat and executeRetreat, since we just need the first hit. Small perf win.

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
Copy link
Contributor

coderabbitai bot commented Feb 22, 2026

Walkthrough

Refactored array-based attack lookup in PlayerImpl by replacing filter(...)[0] with find(...) method, eliminating unnecessary array filtering and directly retrieving the target element. Updated corresponding null checks and method call syntax accordingly.

Changes

Cohort / File(s) Summary
Attack Lookup Optimization
src/core/game/PlayerImpl.ts
Replaced filter()[0] with find() for locating outgoing attacks by id. Updated null checks and adjusted method invocations to work with the direct element rather than array index.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🎯 A filter once searched through the list,
Now find() lands with a single twist,
No more [0] cluttering the way,
Cleaner code wins the day! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and accurately summarizes the main change: replacing filter() with find() for a performance improvement in the retreat functionality.
Description check ✅ Passed The description is related to the changeset, explaining the performance rationale for using find() instead of filter() and confirming completion of testing and documentation requirements.
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

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/core/game/PlayerImpl.ts (1)

369-369: Optional: rename the callback parameter to avoid shadowing the outer attack.

In both methods, the arrow-function parameter is named attack, which shadows the const attack being declared on the same line. It resolves correctly, but renaming it removes the visual confusion.

♻️ Suggested rename
-  const attack = this._outgoingAttacks.find((attack) => attack.id() === id);
+  const attack = this._outgoingAttacks.find((a) => a.id() === id);

Apply the same change on both line 369 and line 377.

Also applies to: 377-377

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/game/PlayerImpl.ts` at line 369, The local constant named attack is
being shadowed by the arrow-function parameter in the calls to
this._outgoingAttacks.find; update the callback parameter name (e.g., change
(attack) => to (outgoing) => or (atk) =>) in the find invocations so the outer
const attack refers to the found item without visual shadowing—apply the same
rename in both occurrences where this._outgoingAttacks.find is used (the lines
creating the const attack and the second similar find).
🤖 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/PlayerImpl.ts`:
- Line 369: The local constant named attack is being shadowed by the
arrow-function parameter in the calls to this._outgoingAttacks.find; update the
callback parameter name (e.g., change (attack) => to (outgoing) => or (atk) =>)
in the find invocations so the outer const attack refers to the found item
without visual shadowing—apply the same rename in both occurrences where
this._outgoingAttacks.find is used (the lines creating the const attack and the
second similar find).

@github-project-automation github-project-automation bot moved this from Triage to Final Review in OpenFront Release Management Feb 23, 2026
@evanpelle evanpelle added this to the v30 milestone Feb 23, 2026
@evanpelle evanpelle merged commit 1d73401 into main Feb 23, 2026
13 of 14 checks passed
@evanpelle evanpelle deleted the retreat-perf branch February 23, 2026 03:54
@github-project-automation github-project-automation bot moved this from Final Review to Complete in OpenFront Release Management Feb 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Performance Performance optimization

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

2 participants