Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

In

switch (ch) {
    case '0': Value v = compute(); v = invert(v); accumulate(v); break;
    case '1': v = compute(); accumulate(v); break;
    default: 
}

the compute and accumulate code pieces are common.

Java allows fall-through cases

Value v = compute();
switch (ch) {
    case '0': v = invert(v); 
    case '1': accumulate(v); break;
    default: 
}

Unfortunately, compute() is executed for ch values outside the {0,1}. Can I share both without the side effects and performance penalty?

By performance penalty I mean that I can always rewrite the switch with more flexible if-then statements.

if (ch == '-') continue; // next loop iteration
assert (ch == '1' or ch == '0');

v = compute();
if (ch == '1') v = invert(v);
accumulate(v);

However, this demands at least one branch statement more than single switch. And, every such statement adds 10x performance slowdown. Yet, I'm gonna use the construction in a loop

for (char ch : computeBits()) // ch can be '0','1','-'
    switch_block;

So, I prefer to stay with a single switch.

share|improve this question
    
Here you have equality test in if with or, so Java assume that if the first equality is thrue, it does not test the second one, so the reference is not for your case. Switch is more heavy code, So you have to test performance of the two algorithms, taking care of JVM launching-time problems. - remove the method : have the bloc of test inside the bloc of for (Garbage Collector incidence) –  cl-r Nov 27 '12 at 10:19
    
Which test cases are you talking about and what it has to do with the reference? –  Val Nov 27 '12 at 10:34
    
I just said that if performances depend on the test inside () - @Kinjal gives you a very good Java solution where switch sequence is replaced by ?: syntaxe –  cl-r Nov 27 '12 at 13:42
    
Kinjal's solution is the same as Corbin's one and similarly has two if tests per valid character. Ternary operator, ?:, is if actually, despite in some respect it may flavour more like datapath rather than control. –  Val Nov 27 '12 at 15:03
add comment

2 Answers

I would consider switching to if statements:

if (ch == '0' || ch == '1') {
    Value v = compute();
    if (ch == '0') {
        v = invert(v);
    }
    accumulate(v);
}

Though this is a bit denser, I find it a lot easier to see the commonality. It took me a few seconds of staring at your switches before I was positive that the only difference was that 0 executes invert and 1 does not.

Also, I tend to think that switch statements should only be used when all expected cases are covered. Considering that you're concerned about when ch is not 1 or 2, it seems that other cases are expected. From looking at your switch, I would not have thought that.

(If other values actually aren't expected, you could throw an exception in the default case to ease debugging if something slips through the cracks.)

share|improve this answer
    
You solution has one big drawback: it has two branches instead of one switch. I have one do-nothing case and may add additional check for exceptions. But this will make things even worse. Branching is very costly in terms of performance as it discards computations on branch misses. Look for "Why is processing a sorted array faster than an unsorted array" for instance. –  Val Nov 27 '12 at 8:38
1  
@Val you're assuming that the prediction will be wrong a lot. Also, if you're going to micro optimize, you should typically state that in your question. My default train of thought here is code maintainability, not extreme performance milking. I would benchmark the switch vs if before assuming it will make a 10x difference in performance. The difference definitely can be 10x, but in this situation, I'd be shocked if it's even a 2x decrease in performance. Also, is this being run million of times in a row? Is this an actual bottleneck? I would guess that it's not. –  Corbin Nov 27 '12 at 9:31
    
I have fixed the definition of my question. Basically, I do not want to make performance testing all the time (performance tests take a lot of time). I want to know if it is possible to refactor the switch without loosing the performance. –  Val Nov 27 '12 at 10:26
1  
@Val If you want to worry about performance at that small of scale, unfortunately testing is the only way to know for sure. For example, there are situations in which ifs can be faster. It's not likely that this is one of those situations, but without profiling you can't know for sure. You should profile anyway though because might be surprised where slow downs are coming from (though I can't really say much with the 20 lines of code given :)). –  Corbin Nov 27 '12 at 10:33
    
Despite quantum mechanics allows that people can pass through walls, I assume that two nested ifs inside a case is definitely costs more pipeline penalties than one case alone. I also can tell without the profiling that slowdowns are coming from everywhere and we should struggle to find the linear code :) –  Val Dec 14 '13 at 17:47
add comment
if (ch != '0' || ch != '1') { return; } 
Value v = (ch == '0') ? invert(compute()) : compute();
accumulate(v);

This will take care of 0 and 1 cases.I am assuming this piece of code runs inside a method where I can have a return for invalid characters.

share|improve this answer
1  
You've made a classic mistake here: if (ch != '0' || ch != '1') will always be true. || should be && –  Simon André Forsberg Dec 14 '13 at 17:27
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.