Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(app): remove some dangerous reverse()s #16591

Merged
merged 1 commit into from
Oct 24, 2024
Merged

fix(app): remove some dangerous reverse()s #16591

merged 1 commit into from
Oct 24, 2024

Conversation

sfoster1
Copy link
Member

@sfoster1 sfoster1 commented Oct 24, 2024

There weer a couple command texts that were doing a .reverse() of the commands array. This is O(N) which is gross and also might be cached which would cause bugs, so replace it with some uses of findLast() that definitely weren't inspired by wanting to do some fun typing exercises.

testing

  • tests pass, really

@sfoster1 sfoster1 requested a review from a team as a code owner October 24, 2024 15:01
@sfoster1 sfoster1 requested review from shlokamin and mjhuff and removed request for a team October 24, 2024 15:01
There weer a couple command texts that were doing a `.reverse()` of the
commands array. This is O(N) which is gross and also might be cached
which would cause bugs, so replace it with some uses of findLast() that
definitely weren't inspired by wanting to do some fun typing exercises.
Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

nice, lgtm!

@sfoster1 sfoster1 merged commit d907591 into edge Oct 24, 2024
37 checks passed
@sfoster1 sfoster1 deleted the fix-more-reverses branch October 24, 2024 19:19
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.

2 participants