Is there a better way to rewrite this ugly switch and if statement combination?

Question

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!

Answer 1

One solution to make it shorter is to sort the crystal values before comparing/switching

int nummin=evList[crysList[0]].crystalNum;
int nummax=evList[crysList[1]].crystalNum;

if (nummin > nummax) 
{
    tmp = nummin;
    nummin = nummax;
    nummax = tmp;
}
// or like Jarod42 said: std::minmax(numin, numax);

if ((nummin == 40 && nummax == 41) ||  // no need to compare the reverse
    (nummin == 42 && nummax == 43))    // and reduce haft of the comparison
{ ... }

Answer 2

A solution combining a 4x4 lookup table and bitwise operations.

Explanation: This is identical for all four cases, so let's look at the case where det=10.

In this case interesting numbers are {40,41,42,43}. If we look at those numbers in their binary representation, a nice pattern shows up.

The upper number is our mask and is computed by det*4. So in this case our mask is10*4=40.

 0b101000 (40)  0b101000       0b101000       0b101000
^0b101000 (40) ^0b101001 (41) ^0b101010 (42) ^0b101011 (43)
=0b000000      =0b000001      =0b000010      =0b000011

After doing the xor (^) between the mask and our allowed numbers, we see that they all have a value in the set {0,1,2,3}. So if e.g. num1 ^ mask < 4, this means, that only the two rightmost bits differ from mask, or num1 is at most 3 larger than mask. If num1 < mask at least some bit(s) left of the two rightmost would flip, and num1 ^ mask would be at least 4. So if num1 ^ mask < 4 is true, num1 lies in {40,41,42,43}.

Furthermore as this lies in {0,1,2,3}, we can also use it as an index in our lookup table.

Now if the previous calculation is true for both num1 and num2, we have a combined index for the lookup table.

int checkDoubleGamma(int det){
    static const int hitTable[4][4] = {
        {Double, Parallel, Perpendicular, Double},
        {Parallel, Double, Double, Perpendicular},
        {Perpendicular, Double, Double, Parallel},
        {Double, Perpendicular, Parallel, Double}
    };

    const int num1 = evList[crysList[0]].crystalNum;
    const int num2 = evList[crysList[1]].crystalNum;

    switch(det) {
    case 10: //0b101000
    case 11: //0b101100
    case 13: //0b110100
    case 14: //0b111000
    {
        const unsigned int mask = 4 * det;
        const unsigned int a = num1 ^ mask;
        if(a < 4){
            const unsigned int b = num2 ^ mask;
            if(b < 4)
                return hitTable[a][b];
        }
        return Double;
    }
    default:
        throw string("made it to default case in checkDoubleGamma switch statement, something is wrong");
        break;
    }
    //Never reaches here
}

Edit: this can be reduced to a 1x4 lookup table

If we look at at the xor-table for a^b, we see that it looks quite similar to our 4x4 lookup table.

 ^ 00 01 10 11
00 00 01 10 11
01 01 00 11 10
10 10 11 00 01 
11 11 10 01 00

This gives us

00,11 = Double
01    = Parallel
10    = Perpendicular 

So we can reduce the old lookup table to a 1x4 lookup table and use a^b as the index.

By looking closer to a^b, we see that it's num1^mask^num2^mask which is equal to num1^num2, which we then will use as the index and save an xor instruction.

This still checks that num2 is within {40,41,42,43}. If num1 only differs from mask in the two rightmost bits, and num2 only differs from num1 in the two rightmost bits, then num2 only differs from mask in the two rightmost bits. So leaving out num2^mask doesn't change behaviour of the program.

int checkDoubleGamma(int det){
    static const int hitTable[4] = {Double, Parallel, Perpendicular, Double};

    const int num1 = evList[crysList[0]].crystalNum;
    const int num2 = evList[crysList[1]].crystalNum;

    switch(det) {
    case 10: //0b101000
    case 11: //0b101100
    case 13: //0b110100
    case 14: //0b111000
    {
        const unsigned int mask = 4 * det;
        const unsigned int a = num1 ^ mask;
        if(a < 4){
            const unsigned int b = num1 ^ num2;
            if(b < 4)
                return hitTable[b];
        }
        return Double;
    }
    default:
        throw string("made it to default case in checkDoubleGamma switch statement, something is wrong");
        break;
    }
    //Never reaches here
}

Answer 3

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;
    }
}