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

Tidy up strToId in Parts.php #685

Merged
merged 1 commit into from Dec 6, 2020
Merged

Tidy up strToId in Parts.php #685

merged 1 commit into from Dec 6, 2020

Conversation

ghost
Copy link

@ghost ghost commented Nov 25, 2020

So I was browsing through this repository once upon a time and I saw that in the function "strToId" in Parts.php that with the Jump Start Hat, instead of having a bunch of "or" conditionals, it just searched through an array. Array $jump_start was declared beforehand to do this, and no such thing occurred with no other hat. So just today while looking through it again I thought "why not do it for all hats, might make it tidier?" So I did. I also added the string "arti" in the checks for the Artifact, 'cuz that's a very common shorthand for the hat.

Now I'm not sure if this is what you'd want. Maybe this is some ancient Jiggy code you didn't want to interfere with for prosperity's sake. Maybe the Jump Start hat must have an array declared when converting a string to its ID or else Fred grows irritable. Honestly, I don't know a lick about PHP - I just wrote this through extrapolation of the logic and syntax. Yes, I checked through the linked site, it works, no syntax errors. From a programmer's point of view, maybe some sort of switch statement would be best for such a function, but I wouldn't know what works for you. Could make it easier for the future, for adding more hats and whatnot. It does look a bit more legible to me but again it's called a "pull request" not a "pull demand". New to this version control stuff, it's really neat.

Regards,
Northadox

@bls1999
Copy link
Collaborator

bls1999 commented Nov 25, 2020

Nice idea! I'm always in the boat for simplifying code. There's also the added bonus of someone other than Jacob or me contributing to this repo. :)

I believe the original purpose of the simple if conditions was for speed. I know in_array is slower than that, but I'm not sure how much slower or if it matters at all. I'll run a few tests and let you know.

@jacob-grahn
Copy link
Owner

Looks good to me 👍

If you want to be even fancier, you could make an array of your arrays and loop through. When you find a match, set $id to the last element of the inner array and break out of the loop.

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.

None yet

2 participants