Простой и понятный способ сравнения трех чисел

У меня есть некоторый код, который имеет последовательность ifs, которая работает, но просто чувствую себя грязно. По сути, я хочу выбрать наибольшее из трех целых чисел и установить флаг статуса, чтобы сказать, какой из них был выбран. Мой текущий код выглядит так:

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;

Этот шаблон встречается несколько раз, и с длинными именами переменных становится немного сложно визуально подтвердить, что каждый из ifних правильный. Я чувствую, что мог бы быть лучший и более ясный способ сделать это; кто-нибудь может предложить что-нибудь?

Есть несколько потенциальных дубликатов, но они не совсем соответствуют этому вопросу.

В предложенном дубликате: Подходы к проверке нескольких условий? Все предложенные решения кажутся такими же неуклюжими, как и исходный код, поэтому они не обеспечивают лучшего решения.

И этот пост Элегантные способы обработки, если (если еще) еще имеет дело только с уровнями вложенности и асимметрией, что не является проблемой здесь.

Факторизовать логику, возвращаться рано

Как предлагается в комментариях, было бы достаточно просто обернуть свою логику в функцию и рано выйти из нее return, чтобы упростить многое. Кроме того, вы можете немного расширить функциональность, делегировав тесты другой функции. Более конкретно:

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

Это короче, чем моя предыдущая попытка:

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

Выше приведено немного более многословно, но легко читается ИМХО и не пересчитывает сравнения несколько раз.

Визуальное подтверждение

В своем ответе вы говорите:

моя проблема была в основном визуальным подтверждением того, что все сравнения использовали одни и те же переменные

... также в своем вопросе вы говорите:

Этот шаблон встречается несколько раз, и с длинными именами переменных становится немного сложно визуально подтвердить, что каждый из них корректен.

Возможно, я не понимаю, чего вы пытаетесь достичь: хотите ли вы копировать и вставлять шаблон везде, где вам это нужно? С помощью функции, подобной приведенной выше, вы фиксируете шаблон один раз и проверяете один раз все, что используют все сравнения a, bи по cмере необходимости. Тогда вам больше не нужно беспокоиться при вызове функции. Конечно, на практике ваша проблема немного сложнее, чем та, которую вы описали: если да, пожалуйста, добавьте некоторые детали, если это возможно.

TL: DR; Ваш код уже правильный и "чистый".

Я вижу, как много людей колеблются вокруг ответа, но всем не хватает леса сквозь деревья. Давайте сделаем полную информатику и математический анализ, чтобы полностью понять этот вопрос.

Сначала отметим, что у нас есть 3 переменные, каждая из которых имеет 3 состояния: <, = или>. Общее количество перестановок составляет 3 ^ 3 = 27 состояний, которым я назначу уникальный номер, обозначаемый P #, для каждого состояния. Этот номер P # является факториальной системой счисления .

Перечисляя все перестановки, которые мы имеем:

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

При осмотре мы видим, что имеем:

• 3 состояния, где A - максимум,
• 3 состояния, где B - максимум,
• 3 состояния, где C - максимум, и
• 4 состояния, где либо A = B, либо B = C.

Давайте напишем программу (см. Сноску) для перечисления всех этих перестановок со значениями для A, B и C. Стабильная сортировка по 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

Если вам интересно, откуда я знаю, какие состояния P # невозможны, теперь вы знаете. :-)

Минимальное количество сравнений для определения порядка:

Log2 (27) = Log (27) / Log (2) = ~ 4,75 = 5 сравнений

т.е. coredump дал правильное 5 минимальное количество сравнений. Я бы отформатировал его код как:

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

Для вашей проблемы мы не заботимся о проверке на равенство, поэтому мы можем опустить 2 теста.

Неважно, насколько чист / плох код, если он получает неправильный ответ, так что это хороший признак того, что вы правильно обрабатываете все случаи!

Далее, что касается простоты, люди продолжают пытаться «улучшить» ответ, где они думают, что улучшение означает «оптимизацию» числа сравнений, но это не совсем то, что вы просите. Вы запутали всех, когда спросили: «Я чувствую, что может быть лучше», но не определили, что означает «лучше». Меньше сравнений? Меньше кода? Оптимальные сравнения?

Теперь, так как вы спрашиваете о читабельности кода (учитывая правильность), я бы сделал одно изменение в вашем коде для удобства чтения: выровняйте первый тест с остальными.

        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

Лично я написал бы это следующим образом, но это может быть слишком необычным для ваших стандартов кодирования:

        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

Сноска. Вот код C ++ для генерации перестановок:

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

Изменения: на основе обратной связи переместил TL: DR наверх, удалил несортированную таблицу, уточнил 27, очистил код, описал невозможные состояния.

@msw сказал вам использовать массив вместо a, b, c, а @Basile сказал вам реорганизовать логику «max» в функцию. Объединение этих двух идей приводит к

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

затем предоставьте функцию, которая вычисляет максимальный индекс произвольного массива:

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

и называть это как

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

Кажется, что общее количество LOC увеличилось по сравнению с исходным, но теперь у вас есть основная логика в многократно используемой функции, и если вы можете многократно использовать эту функцию, она начинает окупаться. Более того, эта FindStrictMaxIndexфункция больше не связана с вашими «бизнес-требованиями» (разделение интересов), поэтому риск, что вам придется изменить ее позже, намного ниже, чем в исходной версии (принцип открытого-закрытого доступа). Например, эту функцию не нужно будет изменять, даже если меняется число аргументов, или нужно использовать другие возвращаемые значения, кроме MOSTLY_ABC, или вы обрабатываете другие переменные, кроме a, b, c. Более того, использование массива вместо 3 различных значений a, b, c может упростить ваш код и в других местах.

Конечно, если во всей вашей программе есть только одно или два места для вызова этой функции, и у вас нет других приложений для хранения значений в массиве, то я бы, вероятно, оставил исходный код как есть (или используйте улучшение Coredump).

Вы, вероятно, должны использовать макрос или функцию, MAX дающую максимум два числа.

Тогда вы просто хотите:

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

Вы могли бы определить

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

но будьте осторожны - особенно в отношении побочных эффектов - при использовании макросов (так как MAX(i++,j--) будет вести себя странно)

Так что лучше определите функцию

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

и использовать его (или хотя бы #define MAX(X,Y) max2ints((X),(Y))....)

Если вам нужно понять происхождение MAX, у вас может быть длинный макрос, такой #define COMPUTE_MAX_WITH_CAUSE(Status,Result,X,Y,Z) как long do{... }while(0) макрос, возможно

#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)

Тогда вы можете вызвать COMPUTE_MAX_WITH_CAUSE(status,res,a,b,c) в нескольких местах. Это немного некрасиво. Я определил локальные переменные x, y, z снизить вредные побочные эффекты ....

Я больше думал об этом, так как моя проблема была в основном визуальным подтверждением того, что все сравнения использовали одни и те же переменные, я думаю, что это может быть полезным подходом:

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? */

То, что каждый макрос берет a, bи cв том же порядке легко подтвердить, и имя макроса избавляет меня от необходимости выяснять, что делают все сравнения и AND.