Is there a better way to rewrite this ugly switch and if statement combination?
c++, if-statement, switch-statement
Solution
I think you can move nearly everything into a simple table and get away with a single table lookup. I haven't studied your conditions in detail, but it looks like something like this will do the job just fine:
// fill the following table in advance using your existing function, or hard-code the
// values if you know they will never change:
ScatterType hitTable[60][60];
ScatterType EventBuffer::checkDoubleHit(int det)
{
// read the crystal Nums once:
unsigned a = evList[cryList[0]].crystalNum;
unsigned b = evList[cryList[1]].crystalNum;
switch(det)
{
case 10:
case 11:
case 13:
case 14:
// better safe than sorry:
assert (a < 60);
assert (b < 60);
return hitTable[a][b];
break;
default:
throw string("made it to default case in checkDoubleHit switch statement, something is wrong");
break;
}
}
Problem
Essentially I have a system of gamma detectors that are segmented into 4 crystals each, in the case when only 2 of the of crystals register a hit we can determine if the pair was perpendicular or parallel to the plane of the reaction generating the gamma-ray. In the process of writing up the logic for this I wound up writing a huge and ugly combination of switch statements which in each detector checks for the combinations of crystal numbers (which are unique across the whole array of detectors and their crystals). Here is the code, including the function in question. ``` //The Parallel and Perpendicular designations are used in addition to the Double //designation for the 90 degree detectors if we get a diagonal scatter in those detectors //then we use the Double designation enum ScatterType{Single, Double, Triple, Quadruple, Parallel, Perpendicular}; ScatterType EventBuffer::checkDoubleGamma(int det) { int num1=evList[crysList[0]].crystalNum; int num2=evList[crysList[1]].crystalNum; switch(det) { case 10: //first of the 90 degree detectors if( (num1==40 && num2==41) || //combo 1 (num1==41 && num2==40) || //combo 1 reverse (num1==42 && num2==43) || //combo 2 (num1==43 && num2==42) )//combo 2 reverse { return Parallel; } else if( (num1==40 && num2==42) || //combo 1 (num1==42 && num2==40) || //combo 1 reverse (num1==41 && num2==43) || //combo 2 (num1==43 && num2==41) )//combo 2 reverse { return Perpendicular; } else { return Double;} break; case 11: //second of the 90 degree detectors if( (num1==44 && num2==45) || //combo 1 (num1==45 && num2==44) || //combo 1 reverse (num1==46 && num2==47) || //combo 2 (num1==47 && num2==46) )//combo 2 reverse { return Parallel; } else if( (num1==44 && num2==47) || //combo 1 (num1==47 && num2==44) || //combo 1 reverse (num1==45 && num2==46) || //combo 2 (num1==46 && num2==45) )//combo 2 reverse { return Perpendicular; } else { return Double;} break; case 13: //third of the 90 degree detectors if( (num1==52 && num2==53) || //combo 1 (num1==53 && num2==52) || //combo 1 reverse (num1==54 && num2==55) || //combo 2 (num1==55 && num2==54) )//combo 2 reverse { return Parallel; } else if( (num1==52 && num2==55) || //combo 1 (num1==55 && num2==52) || //combo 1 reverse (num1==53 && num2==54) || //combo 2 (num1==54 && num2==53) )//combo 2 reverse { return Perpendicular; } else { return Double;} break; case 14: //fourth of the 90 degree detectors if( (num1==56 && num2==57) || //combo 1 (num1==57 && num2==56) || //combo 1 reverse (num1==58 && num2==59) || //combo 2 (num1==59 && num2==58) )//combo 2 reverse { return Parallel; } else if( (num1==56 && num2==59) || //combo 1 (num1==59 && num2==56) || //combo 1 reverse (num1==57 && num2==58) || //combo 2 (num1==58 && num2==57) )//combo 2 reverse { return Perpendicular; } else { return Double;} break; default: throw string("made it to default case in checkDoubleGamma switch statement, something is wrong"); break; } } ``` I am aware that, because the crystal numbers are global rather than per detector, I could do away with the switch statement and have an enormous set of conditionals linked by or statements, essentially reducing things to 3 control paths, one returning Parallel, one returning Perpendicular, and one returning Double, instead of the 12 control paths with 4 of each that I have. I initially wrote it as it is not thinking and in all honesty, thinking about it, this method reduces the average case number of boolean statements to grind through. I just worked out how to make it more efficient by switching on the `evList[crysList[0]].crystalNum` I can reduce the evaluations quite a bit, yielding this: ``` ScatterType EventBuffer::checkDoubleGamma() { int crysNum = crysList[1].crystalNum; switch(evList[crysList[0]].crystalNum) { case 40: if (crysNum == 41) {return Parallel;} else if (crysNum == 42) {return Perpendicular;} else {return Double;} break; case 41: if (crysNum == 40) {return Parallel;} else if (crysNum == 43) {return Perpendicular;} else {return Double;} break; case 42: if (crysNum == 43) {return Parallel;} else if (crysNum == 40) {return Perpendicular;} else {return Double;} break; case 43: if (crysNum == 42) {return Parallel;} else if (crysNum == 41) {return Perpendicular;} else {return Double;} break; case 44: if (crysNum == 45) {return Parallel;} else if (crysNum == 47) {return Perpendicular;} else {return Double;} break; case 45: if (crysNum == 44) {return Parallel;} else if (crysNum == 46) {return Perpendicular;} else {return Double;} break; case 46: if (crysNum == 47) {return Parallel;} else if (crysNum == 45) {return Perpendicular;} else {return Double;} break; case 47: if (crysNum == 46) {return Parallel;} else if (crysNum == 44) {return Perpendicular;} else {return Double;} break; case 52: if (crysNum == 53) {return Parallel;} else if (crysNum == 55) {return Perpendicular;} else {return Double;} break; case 53: if (crysNum == 52) {return Parallel;} else if (crysNum == 54) {return Perpendicular;} else {return Double;} break; case 54: if (crysNum == 55) {return Parallel;} else if (crysNum == 53) {return Perpendicular;} else {return Double;} break; case 55: if (crysNum == 54) {return Parallel;} else if (crysNum == 52) {return Perpendicular;} else {return Double;} break; case 56: if (crysNum == 57) {return Parallel;} else if (crysNum == 59) {return Perpendicular;} else {return Double;} break; case 57: if (crysNum == 56) {return Parallel;} else if (crysNum == 58) {return Perpendicular;} else {return Double;} break; case 58: if (crysNum == 59) {return Parallel;} else if (crysNum == 57) {return Perpendicular;} else {return Double;} break; case 59: if (crysNum == 58) {return Parallel;} else if (crysNum == 56) {return Perpendicular;} else {return Double;} break; default: throw string("made it to default case in checkDoubleGamma switch statement, something is wrong"); break; } } ``` The question still remains though, is there any trick to make this shorter? more efficient? more readable? Thanks in advance!