3 ms·

You are substituting dogma for critical thinking. Sometimes a switch statement (or goto, etc.) is the best overall solution to a problem. In those cases, it i

You are substituting dogma for critical thinking. Sometimes a switch statement (or goto, etc.) is the best overall solution to a problem. In those cases, it is actually good code, not "justifiable code smell."

Sharlin 13y ago
"Code smell" is just a heuristic. It's what happens when you look at some code and something in your head says "this doesn't look good". We all know that in the real world maintainability is more important than minor theoretical performance gains (yes, premature optimization without actually ever measuring the gains is a common thing, as we all hopefully know). I certainly didn't mean to imply that a switch or a goto statement cannot be the best way to implement something; I simply meant that a theoretical performance argument is not enough to make good code out of bad code.
- dalke 13y ago
  A large switch statement is exactly the correct solution for this task. Other virtual machines do the same thing. For example, look at Python's ceval.c, which has a switch table implementation and the text "The traditional bytecode evaluation loop uses a "switch" statement, which decent compilers will optimize as a single indirect branch instruction combined with a lookup table of jump addresses." Actually, it also has a version which uses GCC's "Labels as Values" extension, see http://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html http://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html . The comment then points out that this non-portable solution is up to 15-20% faster than a large switch statement, because having N jump points instead of 1 improves the CPU's branch prediction. As haberman says, this is "actually good code", not "justifiable" bad code. I am another who dislikes the term "code smell." I agree with mbrock - I would rather get a comment which reflects the underlying complaint than use a proxy term like "code smell". In your reply to mbrock you wrote: > the "smell" is just a heuristic that says that without any additional arguments in its favor, a hundred-case switch usually isn't the cleanest and most maintainable way to implement something. Why not just say "a hundred-case switch statement usually isn't ...", and omit a reference to "smell"? What additional meaning does the term "code smell" lend? I dislike using the term "code smell" in general. Some people detest stinky cheeses, or a peaty whisky, while others adore the complex aromas. Often this is a learned taste which comes with age and experience. As a result, the obvious rebuke to any "this is a code smell" comment is "that's because you aren't mature enough to appreciate it." I can't think of any effective response which stays on topic, other than to bring the conversation back to the specific problems in the code. Why not just start from that point instead?
- haberman 13y ago
  I didn't even say anything about performance here. Even ignoring performance, sometimes switch() is best. For example, I wrote a very small JIT compiler for Brainfuck. It uses switch() to do code generation. It's extremely clean and elegant code, if I do say so myself. Breaking apart the switch() with some kind of OO approach would make the program longer and less clear: http://blog.reverberate.org/2012/12/hello-jit-world-joy-of-simple-jits.html http://blog.reverberate.org/2012/12/hello-jit-world-joy-of-s... Do you look at that and think "this doesn't look good" just because it uses switch()? If so, your heuristic could use a little tuning. But now let's talk performance. In interpreter/VM main loops, bytecode dispatch is a huge factor in the overall performance of a VM. This issue isn't "theoretical," "minor," or "premature," it's the result of extensive measurement and experimentation by many people over a large amount of time. If you are not familiar with this well-studied issue, here is some recommended reading: http://software.intel.com/en-us/forums/topic/298506 http://software.intel.com/en-us/forums/topic/298506 http://lua-users.org/lists/lua-l/2011-02/msg00742.html http://lua-users.org/lists/lua-l/2011-02/msg00742.html http://www.cs.tcd.ie/publications/tech-reports/reports.07/TCD-CS-2007-49.pdf http://www.cs.tcd.ie/publications/tech-reports/reports.07/TC... (see section 3.3)
haldean 13y ago
Note that x6502 also uses goto :)

Sharlin 13y ago

"Code smell" is just a heuristic. It's what happens when you look at some code and something in your head says "this doesn't look good". We all know that in the real world maintainability is more important than minor theoretical performance gains (yes, premature optimization without actually ever measuring the gains is a common thing, as we all hopefully know). I certainly didn't mean to imply that a switch or a goto statement cannot be the best way to implement something; I simply meant that a theoretical performance argument is not enough to make good code out of bad code.

dalke 13y ago
A large switch statement is exactly the correct solution for this task. Other virtual machines do the same thing. For example, look at Python's ceval.c, which has a switch table implementation and the text "The traditional bytecode evaluation loop uses a "switch" statement, which decent compilers will optimize as a single indirect branch instruction combined with a lookup table of jump addresses." Actually, it also has a version which uses GCC's "Labels as Values" extension, see http://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html http://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html . The comment then points out that this non-portable solution is up to 15-20% faster than a large switch statement, because having N jump points instead of 1 improves the CPU's branch prediction. As haberman says, this is "actually good code", not "justifiable" bad code. I am another who dislikes the term "code smell." I agree with mbrock - I would rather get a comment which reflects the underlying complaint than use a proxy term like "code smell". In your reply to mbrock you wrote: > the "smell" is just a heuristic that says that without any additional arguments in its favor, a hundred-case switch usually isn't the cleanest and most maintainable way to implement something. Why not just say "a hundred-case switch statement usually isn't ...", and omit a reference to "smell"? What additional meaning does the term "code smell" lend? I dislike using the term "code smell" in general. Some people detest stinky cheeses, or a peaty whisky, while others adore the complex aromas. Often this is a learned taste which comes with age and experience. As a result, the obvious rebuke to any "this is a code smell" comment is "that's because you aren't mature enough to appreciate it." I can't think of any effective response which stays on topic, other than to bring the conversation back to the specific problems in the code. Why not just start from that point instead?
haberman 13y ago
I didn't even say anything about performance here. Even ignoring performance, sometimes switch() is best. For example, I wrote a very small JIT compiler for Brainfuck. It uses switch() to do code generation. It's extremely clean and elegant code, if I do say so myself. Breaking apart the switch() with some kind of OO approach would make the program longer and less clear: http://blog.reverberate.org/2012/12/hello-jit-world-joy-of-simple-jits.html http://blog.reverberate.org/2012/12/hello-jit-world-joy-of-s... Do you look at that and think "this doesn't look good" just because it uses switch()? If so, your heuristic could use a little tuning. But now let's talk performance. In interpreter/VM main loops, bytecode dispatch is a huge factor in the overall performance of a VM. This issue isn't "theoretical," "minor," or "premature," it's the result of extensive measurement and experimentation by many people over a large amount of time. If you are not familiar with this well-studied issue, here is some recommended reading: http://software.intel.com/en-us/forums/topic/298506 http://software.intel.com/en-us/forums/topic/298506 http://lua-users.org/lists/lua-l/2011-02/msg00742.html http://lua-users.org/lists/lua-l/2011-02/msg00742.html http://www.cs.tcd.ie/publications/tech-reports/reports.07/TCD-CS-2007-49.pdf http://www.cs.tcd.ie/publications/tech-reports/reports.07/TC... (see section 3.3)

haldean 13y ago

Note that x6502 also uses goto :)