-
-
Notifications
You must be signed in to change notification settings - Fork 173
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
Filesystem\Mime
: more readable if logic
#6493
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I think this is much harder to read, especially because the first two conditions in each group look the same at first glance and only differ by === false
being present or not.
If we boil it down, it's basically a matrix of the $value
type and the requested $matchWildcard
mode. Maybe we could simplify it by using A::wrap()
to get rid of half of the cases first.
We could further simplify it by using array_filter()
. Its callback can then use early returns of true/false
so that we can use one level of separate ifs.
I disagree that it's harder to read. It takes such a mental load down in the second else clause to keep track of which conditions actually apply there. With that, this new explicitness would help a lot. I agree that this still isn't "the yellow from the egg". I will have a stab at your suggestions, would be indeed great to improve this more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better 👍
Co-authored-by: Lukas Bestle <[email protected]>
Description
Summary of changes
Filesystem\Mime::toExtensions()
: refactored nested if-else into one-level ifReasoning
The nested if-else logic is super hard to read and keep track of the conditions for each result, which also does the same. Changing it to a one-level if is a lot easier to read, which various conditions lead to adding an extension, even if we have to list some conditions multiple times then.
Ready?