Prosty i przejrzysty sposób porównywania trzech liczb

Mam kod, który ma sekwencję ifs, która działa, ale po prostu niechlujny. Zasadniczo chcę wybrać największą z trzech liczb całkowitych i ustawić flagę statusu, aby powiedzieć, która została wybrana. Mój obecny kod wygląda następująco:

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;

Ten wzór występuje kilka razy, a przy długich nazwach zmiennych trudno jest wizualnie potwierdzić, że każda z nich ifjest poprawna. Wydaje mi się, że może być lepszy i jaśniejszy sposób na zrobienie tego; czy ktoś może coś zasugerować?

Istnieje kilka potencjalnych duplikatów, ale nie do końca odpowiadają temu pytaniu.

W sugerowanym duplikacie: Podejścia do sprawdzania wielu warunków? wszystkie sugerowane rozwiązania wydają się równie nieporadne jak oryginalny kod, więc nie zapewniają lepszego rozwiązania.

I ten post Eleganckie sposoby radzenia sobie, jeśli (jeśli inaczej) dotyczą tylko poziomów zagnieżdżenia i asymetrii, co nie jest tutaj problemem.

Rozłóż logikę na czynniki, wróć wcześniej

Jak sugerowano w komentarzach, wystarczy po prostu zawinąć logikę w funkcję i wyjść wcześniej z return's', aby znacznie uprościć sprawę . Można również rozłożyć na czynniki trochę funkcjonalność, delegując testy do innej funkcji. Bardziej konkretnie:

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

To jest krótsze niż moja poprzednia próba:

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

Powyższe jest nieco bardziej szczegółowe, ale jest łatwe do odczytania IMHO i nie oblicza porównań wiele razy.

Potwierdzenie wizualne

W swojej odpowiedzi mówisz:

moim problemem było głównie wizualne potwierdzenie, że wszystkie porównania wykorzystywały te same zmienne

... również w swoim pytaniu mówisz:

Ten wzorzec występuje kilka razy, a przy długich nazwach zmiennych trudno jest wizualnie potwierdzić, że każdy z nich jest poprawny.

Mogę nie rozumieć, co próbujesz osiągnąć: czy chcesz skopiować i wkleić wzór wszędzie tam, gdzie go potrzebujesz? Z funkcji takich jak ten powyżej, przechwytywanie wzorca raz i sprawdzić raz na zawsze, że wszystkie porównania użyć a, ba cw razie potrzeby. Następnie nie musisz się już martwić, kiedy wywołujesz tę funkcję. Oczywiście, może w praktyce twój problem jest nieco bardziej złożony niż opisany przez Ciebie: jeśli tak, proszę dodaj szczegóły, jeśli to możliwe.

TL: DR; Twój kod jest już poprawny i „czysty”.

Widzę wielu ludzi wahających się wokół odpowiedzi, ale wszyscy tęsknią za lasem przez drzewa. Zróbmy pełną informatykę i analizę matematyczną, aby całkowicie zrozumieć to pytanie.

Po pierwsze, zauważamy, że mamy 3 zmienne, każda z 3 stanami: <, = lub>. Całkowita liczba permutacji wynosi 3 ^ 3 = 27 stanów, do których przypisam unikalny numer, oznaczony P #, dla każdego stanu. Ten numer P # to system liczb czynnikowych .

Zliczając wszystkie permutacje, które mamy:

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

Po kontroli stwierdzamy, że mamy:

• 3 stany, w których A jest wartością maksymalną,
• 3 stany, w których B jest maksimum,
• 3 stany, w których C jest wartością maksymalną, oraz
• 4 stany, w których A = B lub B = C.

Napiszmy program (patrz przypis), aby wyliczyć wszystkie te permutacje wartościami A, B i C. Sortowanie stabilne według 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

Jeśli zastanawiasz się, skąd wiedziałem, które stany P # są niemożliwe, teraz już wiesz. :-)

Minimalna liczba porównań w celu ustalenia kolejności wynosi:

Log2 (27) = Log (27) / Log (2) = ~ 4,75 = 5 porównań

tzn. rdzeń rdzeniowy dał prawidłową 5 minimalną liczbę porównań. Sformatowałbym jego kod jako:

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

W przypadku Twojego problemu nie zależy nam na testowaniu równości, więc możemy pominąć 2 testy.

Nie ma znaczenia, jak czysty / zły jest kod, jeśli otrzyma złą odpowiedź, więc jest to dobry znak, że poprawnie załatwiasz wszystkie sprawy!

Następnie, jeśli chodzi o prostotę, ludzie próbują „poprawić” odpowiedź, w której ich zdaniem poprawa oznacza „optymalizację” liczby porównań, ale nie jest to dokładnie to, o co prosisz. Zdezorientowałeś wszystkich, gdy zapytałeś „Czuję, że może być lepszy”, ale nie zdefiniowałeś, co oznacza „lepszy”. Mniej porównań? Mniej kodu? Optymalne porównania?

Teraz, gdy pytasz o czytelność kodu (biorąc pod uwagę poprawność), dokonałbym tylko jednej zmiany w twoim kodzie dla czytelności: Dopasuj pierwszy test do pozostałych.

        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

Osobiście napisałbym to w następujący sposób, ale może to być zbyt niekonwencjonalne dla twoich standardów kodowania:

        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

Przypis: Oto kod C ++ do generowania permutacji:

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

Edycje: Na podstawie opinii, przeniesiono TL: DR na górę, usunięto nieposortowaną tabelę, wyjaśniono 27, wyczyściłem kod, opisałem niemożliwe stany.

@msw powiedział ci, abyś użył tablicy zamiast a, b, c, a @Basile powiedział ci, aby przełożyć logikę „max” na funkcję. Połączenie tych dwóch pomysłów prowadzi do

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

następnie podaj funkcję, która oblicza maksymalny indeks dowolnej tablicy:

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

i nazwać to tak

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

Wydaje się, że całkowita liczba LOC wzrosła w porównaniu z pierwotną, ale teraz masz podstawową logikę w funkcji wielokrotnego użytku, a jeśli możesz wielokrotnie użyć tej funkcji, zacznie się to opłacać. Co więcej, FindStrictMaxIndexfunkcja nie jest już powiązana z twoimi „wymaganiami biznesowymi” (oddzielenie problemów), dlatego ryzyko, które będziesz musiał zmodyfikować później, jest znacznie niższe niż w oryginalnej wersji (zasada otwartego zamknięcia). Na przykład ta funkcja nie będzie musiała zostać zmieniona, nawet jeśli liczba argumentów ulegnie zmianie, lub będzie trzeba użyć innych zwracanych wartości niż MOSTLY_ABC, lub jeśli przetwarzane są inne zmienne niż a, b, c. Ponadto użycie tablicy zamiast 3 różnych wartości a, b, c może uprościć kod również w innych miejscach.

Oczywiście, jeśli w całym twoim programie jest tylko jedno lub dwa miejsca do wywołania tej funkcji i nie masz żadnych innych aplikacji do przechowywania wartości w tablicy, prawdopodobnie zostawiłbym oryginalny kod w takiej postaci, w jakiej jest (lub użyj @ poprawa rdzeni).

Prawdopodobnie powinieneś użyć makra lub funkcji MAX podającej maksymalnie dwie liczby.

Więc po prostu chcesz:

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

Mogłeś zdefiniować

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

ale MAX(i++,j--) zachowaj ostrożność - szczególnie przy skutkach ubocznych - podczas korzystania z makr (ponieważ zachowywałby się dziwnie)

Więc lepiej zdefiniuj funkcję

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

i użyj go (lub przynajmniej #define MAX(X,Y) max2ints((X),(Y))...)

Jeśli chcesz zrozumieć pochodzenie MAX, możesz mieć długie makro, #define COMPUTE_MAX_WITH_CAUSE(Status,Result,X,Y,Z) które może być długim do{... }while(0) makro

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

Następnie możesz wywoływać COMPUTE_MAX_WITH_CAUSE(status,res,a,b,c) w kilku miejscach. To jest trochę brzydkie. I zdefiniowane zmienne lokalne x, y, z aby obniżyć złych skutków ubocznych ....

Zastanowiłem się nad tym, więc ponieważ mój problem był głównie wizualnym potwierdzeniem, że wszystkie porównania wykorzystują te same zmienne, myślę, że może to być przydatne podejście:

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

Że każde makro trwa a, bi cw tej samej kolejności jest łatwe, aby potwierdzić, a nazwa makra oszczędza mi mający wypracować co wszystkie porównania i łączeniu robią.