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

converting else-if to switch-case (lines 42-79) #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

converting else-if to switch-case (lines 42-79) #22

wants to merge 1 commit into from

Conversation

elhayra
Copy link

@elhayra elhayra commented Nov 13, 2016

No description provided.

@pawelsky
Copy link

Would you mind clarifying what is the real benefit of this change?

@elhayra
Copy link
Author

elhayra commented Nov 14, 2016

The compiler is better in optimizing a switch-statement than an if-statement.
Also, I think the code is slightly more readable this way.

@pawelsky
Copy link

I was more interested in REAL benefits :)

How much time do you gain by doing that - have you measured? How does it affect code size - did you notice that it is bigger with switch-case, than if-else? Also is it really worth to do optimizations (if this actually brings any optimization at all) in a method which in most of the cases will be called only once at the beginning of the code?

@collin80
Copy link

Butting in as I'm soon to want to push some commits so I'm looking at this code.

The efficiency is meaningless as the difference will be on the order of microseconds and it is only done once upon start up. But, as far as code aesthetics are concerned, it is generally advisable to use switch when there are more than 2-3 options. The switch/case version "looks nicer" which is its only advantage. But, it doesn't hurt anything so I wouldn't see any reason not to incorporate such a change.

@elhayra
Copy link
Author

elhayra commented Nov 14, 2016

I agree. This change is only "cosmetic".
I admit I didn't noticed this code only runs once on startup, so performance wise, this is indeed meaningless.
I still think this minor change is more readable and maintainable, but I will understand if you choose not to incorporate it.

@pawelsky
Copy link

Readability is more matter of a personal preference (for me if-elses look cleaner), and given the change slightly increases the resulting image size (so it does hurt a bit) I don't find it worth it. Just an opinion...

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.

3 participants