search

teachop / FlexCAN_Library

Arduino library for CAN on Teensy 3.1
The Unlicense
155 stars 122 forks source link

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

Open elhayra opened 7 years ago

pawelsky commented 7 years ago

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

elhayra commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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...