Simple and clean way of comparing three numbers

Question

I've got some code that has a sequence of ifs that work, but just feel messy. Basically, I want to choose the largest of three integers and set a status flag to say which was chosen. My current code looks like this:

a = countAs();
b = countBs();
c = countCs();

if (a > b && a > c)
    status = MOSTLY_A;
else if (b > a && b > c)
    status = MOSTLY_B;
else if (c > a && c > b)
    status = MOSTLY_C;
else
    status = DONT_KNOW;

This pattern occurs a few times, and with long variable names it gets a little difficult to visually confirm that each if is correct. I feel there might be a better and clearer way to do this; can anyone suggest anything?

There are some potential duplicates, but they don't quite align with this question.

In the suggested duplicate: Approaches to checking multiple conditions? all of the suggested solutions seem equally clumsy as the original code, so they do not provide a better solution.

And this post Elegant ways to handle if(if else) else deals only with nesting levels and asymmetry, which is not the problem here.

score 13 · Accepted Answer · edited Apr 12 '17 at 07:31

Factorize logic, return early

As suggested in comments, it would be sufficient to simply wrap your logic in a function and exit early with return's in order to simplify things a lot. Also, you could factorize a bit of functionnality by delegating tests to another function. More concretely:

bool mostly(max,u,v) {
   return max > u && max > v;
}

status_t strictly_max_3(a,b,c)
{
  if mostly(a,b,c) return MOSTLY_A;
  if mostly(b,a,c) return MOSTLY_B;
  if mostly(c,a,b) return MOSTLY_C;
  return DONT_KNOW;
}

This is shorter than my previous attempt:

status_t index_of_max_3(a,b,c)
{
  if (a > b) {
    if (a == c)
      return DONT_KNOW;
    if (a > c)
      return MOSTLY_A;
    else
      return MOSTLY_C;
  } else {
    if (a == b)
      return DONT_KNOW;
    if (b > c)
      return MOSTLY_B;
    else
      return MOSTLY_C;
  }
}

The above is a little more verbose, but is easy to read IMHO and does not recompute comparisons multiple times.

Visual confirmation

In your answer you say:

my issue was mostly visual confirmation that all comparisons used the same variables

... also, in your question, you say:

This pattern occurs a few times, and with long variable names it gets a little difficult to visually confirm that each if is correct.

I may not understand what you are trying to achieve: do you want to copy-paste the pattern everywhere you need it? With a function like the one above, you capture the pattern once, and you check once for all that all comparisons use a, b and c as required. Then, you don't need to worry anymore when you call the function. Of course, maybe in practice your problem is a little more complex than the one you described: if so, please add some details if possible.

Michaelangel007 · Answer 2 · 2015-06-24T16:36:22.217

TL:DR; Your code is already correct and "clean".

I see a lot of people waffling around the answer but everyone is missing the forest through the trees. Let's do the full computer science and mathematical analysis to completely understand this question.

First, we note that we have 3 variables, each with 3 states: <, =, or >. The total number of permutations is 3^3 = 27 states, which I'll assign an unique number, denoted P#, for each state. This P# number is a factorial number system.

Enumerating all the permutations we have:

a ? b | a ? c | b ? c |P#| State
------+-------+-------+--+------------
a < b | a < c | b < c | 0| C
a = b | a < c | b < c | 1| C
a > b | a < c | b < c | 2| C
a < b | a = c | b < c | 3| impossible a<b b<a
a = b | a = c | b < c | 4| impossible a<a
a > b | a = c | b < c | 5| A=C > B
a < b | a > c | b < c | 6| impossible a<c a>c
a = b | a > c | b < c | 7| impossible a<c a>c
a > b | a > c | b < c | 8| A
a < b | a < c | b = c | 9| B=C > A
a = b | a < c | b = c |10| impossible a<a
a > b | a < c | b = c |11| impossible a<c a>c
a < b | a = c | b = c |12| impossible a<a
a = b | a = c | b = c |13| A=B=C
a > b | a = c | b = c |14| impossible a>a
a < b | a > c | b = c |15| impossible a<c a>c
a = b | a > c | b = c |16| impossible a>a
a > b | a > c | b = c |17| A
a < b | a < c | b > c |18| B
a = b | a < c | b > c |19| impossible b<c b>c
a > b | a < c | b > c |20| impossible a<c a>c
a < b | a = c | b > c |21| B
a = b | a = c | b > c |22| impossible a>a
a > b | a = c | b > c |23| impossible c>b b>c
a < b | a > c | b > c |24| B
a = b | a > c | b > c |25| A=B > C
a > b | a > c | b > c |26| A

By inspection we see we have:

3 states where A is the max,
3 states where B is the max,
3 states where C is the max, and
4 states where either A=B, or B=C.

Let's write a program (see the footnote) to enumerate all these permutations with values for A, B, and C. Stable sorting by P#:

a ?? b | a ?? c | b ?? c |P#| State
1 <  2 | 1 <  3 | 2 <  3 | 0| C
1 == 1 | 1 <  2 | 1 <  2 | 1| C
1 == 1 | 1 <  3 | 1 <  3 | 1| C
2 == 2 | 2 <  3 | 2 <  3 | 1| C
2  > 1 | 2 <  3 | 1 <  3 | 2| C
2  > 1 | 2 == 2 | 1 <  2 | 5| ??
3  > 1 | 3 == 3 | 1 <  3 | 5| ??
3  > 2 | 3 == 3 | 2 <  3 | 5| ??
3  > 1 | 3  > 2 | 1 <  2 | 8| A
1 <  2 | 1 <  2 | 2 == 2 | 9| ??
1 <  3 | 1 <  3 | 3 == 3 | 9| ??
2 <  3 | 2 <  3 | 3 == 3 | 9| ??
1 == 1 | 1 == 1 | 1 == 1 |13| ??
2 == 2 | 2 == 2 | 2 == 2 |13| ??
3 == 3 | 3 == 3 | 3 == 3 |13| ??
2  > 1 | 2  > 1 | 1 == 1 |17| A
3  > 1 | 3  > 1 | 1 == 1 |17| A
3  > 2 | 3  > 2 | 2 == 2 |17| A
1 <  3 | 1 <  2 | 3  > 2 |18| B
1 <  2 | 1 == 1 | 2  > 1 |21| B
1 <  3 | 1 == 1 | 3  > 1 |21| B
2 <  3 | 2 == 2 | 3  > 2 |21| B
2 <  3 | 2  > 1 | 3  > 1 |24| B
2 == 2 | 2  > 1 | 2  > 1 |25| ??
3 == 3 | 3  > 1 | 3  > 1 |25| ??
3 == 3 | 3  > 2 | 3  > 2 |25| ??
3  > 2 | 3  > 1 | 2  > 1 |26| A

In case you were wondering how I knew which P# states were impossible, now you know. :-)

The minimum number of comparisons to determine the order is:

Log2(27) = Log(27)/Log(2) = ~4.75 = 5 comparisons

i.e. coredump gave the correct 5 minimal number of comparisons. I would format his code as:

status_t index_of_max_3(a,b,c)
{
    if (a > b) {
        if (a == c) return DONT_KNOW; // max a or c
        if (a >  c) return MOSTLY_A ;
        else        return MOSTLY_C ;
    } else {
        if (a == b) return DONT_KNOW; // max a or b
        if (b >  c) return MOSTLY_B ;
        else        return MOSTLY_C ;
    }
}

For your problem we don't care about testing for equality so we can omit 2 tests.

It doesn't matter how clean/bad the code is if it gets the wrong answer so this is a good sign that you are handling all the cases correctly!

Next, as for simplicity, people keep trying to "improve" the answer, where they think improving means "optimizing" the number of comparisons, but that isn't strictly what you are asking. You confused everyone where you asked "I feel there might be a better" but didn't define what 'better' means. Less comparisons? Less code? Optimal comparisons?

Now since you are asking about code readability (given correctness) I would only make one change to your code for readability: Align the first test with the others.

        if      (a > b && a > c)
            status = MOSTLY_A;
        else if (b > a && b > c)
            status = MOSTLY_B;
        else if (c > a && c > b)
            status = MOSTLY_C;
        else
            status = DONT_KNOW; // a=b or b=c, we don't care

Personally I would write it the following way but this may be too unorthodox for your coding standards:

        if      (a > b && a > c) status = MOSTLY_A ;
        else if (b > a && b > c) status = MOSTLY_B ;
        else if (c > a && c > b) status = MOSTLY_C ;
        else /*  a==b  || b ==c*/status = DONT_KNOW; // a=b or b=c, we don't care

Footnote: Here is the C++ code to generate the permutations:

#include <stdio.h>

char txt[]  = "< == > ";
enum cmp      { LESS, EQUAL, GREATER };
int  val[3] = { 1, 2, 3 };

enum state    { DONT_KNOW, MOSTLY_A, MOSTLY_B, MOSTLY_C };
char descr[]= "??A B C ";

cmp Compare( int x, int y ) {
    if( x < y ) return LESS;
    if( x > y ) return GREATER;
    /*  x==y */ return EQUAL;
}

int main() {
    int i, j, k;
    int a, b, c;

    printf( "a ?? b | a ?? c | b ?? c |P#| State\n" );
    for( i = 0; i < 3; i++ ) {
        a = val[ i ];
        for( j = 0; j < 3; j++ ) {
            b = val[ j ];
            for( k = 0; k < 3; k++ ) {
                c = val[ k ];

                int cmpAB = Compare( a, b );
                int cmpAC = Compare( a, c );
                int cmpBC = Compare( b, c );
                int n     = (cmpBC * 9) + (cmpAC * 3) + cmpAB; // Reconstruct unique P#

                printf( "%d %c%c %d | %d %c%c %d | %d %c%c %d |%2d| "
                    , a, txt[cmpAB*2+0], txt[cmpAB*2+1], b
                    , a, txt[cmpAC*2+0], txt[cmpAC*2+1], c
                    , b, txt[cmpBC*2+0], txt[cmpBC*2+1], c
                    , n
                );

                int status;
                if      (a > b && a > c) status = MOSTLY_A;
                else if (b > a && b > c) status = MOSTLY_B;
                else if (c > a && c > b) status = MOSTLY_C;
                else /*  a ==b || b== c*/status = DONT_KNOW; // a=b, or b=c

                printf( "%c%c\n", descr[status*2+0], descr[status*2+1] );
            }
        }
    }
    return 0;
}

Edits: Based on feedback, moved TL:DR to top, removed unsorted table, clarified 27, cleaned up code, described impossible states.

Doc Brown · Answer 3 · 2015-06-24T15:17:07.480

@msw told you to use an array instead of a,b,c, and @Basile told you refactor the "max" logic into a function. Combining these two ideas leads to

val[0] = countAs();    // in the real code, one should probably define 
val[1] = countBs();    // some enum for the indexes 0,1,2 here
val[2] = countCs();

 int result[]={DONT_KNOW, MOSTLY_A, MOSTLY_B, MOSTLY_C};

then provide a function which calculates the max index of an arbitrary array:

// returns the index of the strict maximum, and -1 when the maximum is not strict
int FindStrictMaxIndex(int *values,int arraysize)
{
    int maxVal=INT_MIN;
    int maxIndex=-1;
    for(int i=0;i<arraysize;++i)
    {
       if(values[i]>maxVal)
       {
         maxVal=values[i];
         maxIndex=i;
       }
       else if (values[i]==maxVal)
       {
         maxIndex=-1;
       }
    }
    return maxIndex;
}

and call it like

 return result[FindStrictMaxIndex(val,3)+1];

The total number of LOC seems to have increased over the original one, but now you have the core logic in a reusable function, and if you can reuse the function multiple times, it starts to pay off. Moreover, the FindStrictMaxIndex function is not interwoven with your "business requirements" any more (separation of concerns), thus the risk you will have to modify it later is much lower than in your original version (open-closed principle). For example, that function will not have to be changed even if the number of arguments changes, or need to use other return values than MOSTLY_ABC, or you are processing other variables than a,b,c. Moreover, the use of an array instead of 3 different values a, b, c might simplify your code in other places as well.

Of course, if in your whole program there are only one or two places for calling this function, and you do not have any further applications for holding the values in an array, then I would probably leave the original code as it is (or use @coredump's improvement).

Basile Starynkevitch · Answer 4 · 2015-06-22T05:20:40.023

You probably should use a macro or a function MAX giving the maximum of two numbers.

Then you just want:

 status = MAX(a,MAX(b,c));

You might have defined

 #define MAX(X,Y) (((X)>(Y))?(X):(Y))

but be cautious -notably about side effects- when using macros (since MAX(i++,j--) would behave strangely)

So better define a function

 static inline int max2ints(int x, int y) {
    return (x>y)?x:y;
 }

and use it (or at least #define MAX(X,Y) max2ints((X),(Y)) ....)

If you need to understand the origin of the MAX you might have a long macro like #define COMPUTE_MAX_WITH_CAUSE(Status,Result,X,Y,Z) which is a long do{ ... }while(0) macro, perhaps

#define COMPUTE_MAX_WITH_CAUSE(Status,Result,X,Y,Z) do { \
  int x= (X), y= (Y), z=(Z); \
  if (x > y && y > z) \
    { Status = MOSTLY_FIRST; Result = x; } \
  else if (y > x && y > z) \
    { Status = MOSTLY_SECOND; Result = y; } \
  else if (z > x && z > y) \
    { Status = MOSTLY_THIRD; Result = z; } \
  /* etc */ \
  else { Status = UNKNOWN; Result = ... } \
} while(0)

Then you could invoke COMPUTE_MAX_WITH_CAUSE(status,res,a,b,c) at several places. It is a bit ugly. I defined local variables x, y, z to lower bad side effects....

score -1 · Answer 5 · answered Jun 23 '15 at 05:49

I've had more of a think about this, so since my issue was mostly visual confirmation that all comparisons used the same variables, I think this might be a useful approach:

a = countAs();
b = countBs();
c = countCs();

if (FIRST_IS_LARGEST(a, b, c))
    status = MOSTLY_A;
else if (SECOND_IS_LARGEST(a, b, c))
    status = MOSTLY_B;
else if (THIRD_IS_LARGEST(a, b, c))
    status = MOSTLY_C;
else
    status = DONT_KNOW; /* NO_SINGLE_LARGEST is a better name? */

That each macro takes a, b and c in the same order is easy to confirm, and the macro name saves me having to work out what all the comparisons and ANDs are doing.

Simple and clean way of comparing three numbers

5 Answers5

Factorize logic, return early

Visual confirmation