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

T-Buttons: small efficiency boost #1006

Merged
merged 9 commits into from
Oct 27, 2023
Merged

T-Buttons: small efficiency boost #1006

merged 9 commits into from
Oct 27, 2023

Conversation

mognify
Copy link
Contributor

@mognify mognify commented Nov 19, 2022

tried to make it more legible and efficient
so i made it more complicated =)

changed the chained if-elseif to table lookup
due to integers being used and not strings for the index/conditional check

im still new to lua so feel free to roast me, im tryna learn, coming from java
@mognify
Copy link
Contributor Author

mognify commented Nov 19, 2022

i just realized i left the recv variable outside the if-block that uses it............. :\

also idk how/if garbage collection works in lua, but i shouldve set recv to nil afterwards to save memory (maybe?)

@mognify
Copy link
Contributor Author

mognify commented Dec 12, 2022

i just realized the first If true = do a bunch of stuff should be if false = return...

Copy link
Member

@Histalek Histalek left a comment

Choose a reason for hiding this comment

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

All in all i like this change. Imo it makes the code more readable by using a lookup table and the ternary operator.

In regards to the code comments: I personally would remove them, as they don't really add more to what the code itself already tells us.

Regarding requested changes, please do address your own comments on this pull request, they are definitely valid :)

i dont think RECEIVE_ALL is gonna have a fun time. It probably is going to fail to get called...
but according to something i read off google: Nil isnt actually nothing, it's just an absolutely unique property value
so this should be fine. Coinflippin it
@mognify mognify temporarily deployed to histalek-dev-env December 14, 2022 04:30 — with GitHub Actions Inactive
Copy link
Member

@TimGoll TimGoll left a comment

Choose a reason for hiding this comment

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

I like this, thanks a lot. But please address the points raised by @Histalek

@saibotk saibotk requested a review from Histalek January 3, 2023 22:03
@stale
Copy link

stale bot commented Mar 25, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 25, 2023
@Histalek Histalek added accepted and removed stale labels Mar 25, 2023
@mognify
Copy link
Contributor Author

mognify commented Mar 25, 2023

sorry, i had a bunch of things taking up my time and energy
i can refine this and update now (real quick (soon (tm)))

@Histalek
Copy link
Member

sorry, i had a bunch of things taking up my time and energy i can refine this and update now (real quick (soon (tm)))

No worries!

(i probably need to reconfigure the stale bot to only annoy us Team-Members or remove it altogether ...)

@saibotk saibotk added the skip-changelog Skips the requirement for an entry in the CHANGELOG.md label Oct 27, 2023
@saibotk saibotk changed the title small efficiency boost T-Buttons: small efficiency boost Oct 27, 2023
@saibotk saibotk enabled auto-merge (squash) October 27, 2023 23:50
@saibotk saibotk merged commit 95e4624 into TTT-2:master Oct 27, 2023
3 checks passed
TimGoll added a commit that referenced this pull request Dec 12, 2023
saibotk pushed a commit that referenced this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog Skips the requirement for an entry in the CHANGELOG.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants