11

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.

  • 3
    To me the only problem is the code repetition. What you have here is extremely clear to read, why change that? Just break it out into a function that takes in a,b,c and returns status. If you it makes you feel better, drop an "inline" on it. No macros, no complexity, just good old function extraction. I will thank you for your clear code if I need to work with it later. – J Trana Jun 22 '15 at 5:23
  • possible duplicate of Approaches to checking multiple conditions? – gnat Jun 22 '15 at 5:51
  • see also: Elegant ways to handle if(if else) else – gnat Jun 22 '15 at 5:52
  • Note that your #defines are named poorly. Consider a = 40, b = 30, c = 30. The result is MOSTLY_A. But most of the things are actually not A, but B or C. You might want to try MORE_A (although still ambiguous - more A than B and more A than C, or more A than B and C combined?), or A_LARGEST, MAX_IS_A, ...? (which also suggests max(a, max(b, c)) as an answer to the question). – tony Jun 23 '15 at 16:13
12

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.

  • 1
    I don't understand your comment about DONT_KNOW , what if c is the smallest and a and b are the same? The algorithm would return that b is the biggest, while a is the same as b. – Pieter B Jun 22 '15 at 9:57
  • @PieterB I was wronlgy assuming that it would not matter if we returned either MOSTLY_A or MOSTLY_C in case a == c and a > b. This is fixed. Thanks. – coredump Jun 22 '15 at 12:10
  • @DocBrown Granted, most of the benefits come from the early exit behavior. – coredump Jun 22 '15 at 14:13
  • 1
    +1, now it is indeed an improvement over the OPs original code. – Doc Brown Jun 23 '15 at 5:23
8

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.

  • -1: wouldn't reducing the number of decisions lead to simpler code paths and more readable code? You argument is not clear: first, you say everybody is wrong; then, you put not one or two but three tables; I hoped they would lead to a simpler way to compute the result but instead you confirmed what everybody else already knew (OP's code does the right thing). Sure, the question is about readability, but readability is not achieved only by modifying code layout (you admit your changes would hardly fit existing code standards). It makes sense to simplify logic when optimizing for readability. – coredump Jun 24 '15 at 8:03
  • More constructively: I'd suggest to simplify your answer by leaving out some details and thinking about the structure of your answer. I appreciate that you took time to write and post the C++ code generating permutations, but maybe you could give the main result and only one table: as it is now, it looks like you dumped all your work as-is. I almost failed to spot the TL;DR thing (you could start with that). Hope it helps. – coredump Jun 24 '15 at 8:09
  • 2
    Thanks for the constructive feedback coredump. I've removed the middle unsorted table since it is easily verified. – Michaelangel007 Jun 24 '15 at 16:16
  • 2
    Jesus Christ! Who would say comparing three numbers is almost as complex as rocket science? – Mandrill Jun 24 '15 at 17:49
  • @Mandrill As computer scientists it is our job to understand a problem thoroughly. Only by enumerating all 27 possible permutations for a 3-way comparison can we verify that our solution works in ALL cases. The last thing we want as programmers are hidden bugs and unaccounted edge cases. Verbosity is the price one pays for correctness. – Michaelangel007 Jul 9 '15 at 17:15
5

@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).

  • I like that - the guts of FindStrictMaxIndex() may not be too clean, but from the caller point of view it is reasonably obvious what is trying to be achieved. – Ken Y-N Jun 22 '15 at 5:24
  • Or instead of holding two arrays, hold one array of key-value-pairs: { MOSTLY_A, countAs() }, take the first element ordered by value and read off the key. – Julia Hayward Jun 22 '15 at 8:36
  • @JuliaHayward: the main reason I did not suggest such a solution was the "C" tag of the question - in C, one will need some more boilerplate code to deal with key-value pairs, and creating a function typed in terms of KVPs will probably be not as reusable in different contexts than a simple int typed function. But I agree to your comment if one uses a different language like Python or Perl. – Doc Brown Jun 22 '15 at 11:11
  • 1
    @gnasher729: it depends on how many "duplicates" in the orginal code are, how similar they really are, and how often the FindStrictMaxIndex function can be reused. For one or just two times of reusage, this won't pay off, of course, but that is what I wrote already in my answer. Note also the other advantages I mentioned above concerning future changes. – Doc Brown Jun 23 '15 at 9:26
  • 1
    ... and note that the original 8 lines can be replaced by a simple one-liner return result[FindStrictMaxIndex(val,3)]; at the point in code where the original 8 lines were placed. The other parts, especially FindStrictMaxIndex itself are completely separated from the "business logic", which moves them out of the focus of changing business requirements. – Doc Brown Jun 23 '15 at 9:38
-1

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....

  • 2
    Refactoring the common logic into a function is the right approach, but I would actually avoid two things here: 1. I would not "invent" new requirements (the OP did not ask for calculating the maximum). And 2nd: even if the resulting code may become more DRY, it is very debatable if this justifies a complicated macro. – Doc Brown Jun 22 '15 at 7:33
  • 1
    Macros should be a tool of last resort. Definitely out of bounds for this problem. – kevin cline Jun 23 '15 at 7:31
-1

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.

  • 1
    (1) why auxiliary macros instead of functions? (2) why do you need visual confirmation here? is it really your core problem or is the need for visual confirmation a consequence of code duplication? Your best option is to factorize your code into a single, simple function that you check once for all. – coredump Jun 24 '15 at 13:44

protected by Community Jun 23 '15 at 13:05

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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