Modo semplice e pulito per confrontare tre numeri

Factorize logic, return early

Come suggerito nei commenti, sarebbe sufficiente avvolgere semplicemente la logica in una funzione e uscire presto con return per semplificare molto le cose. Inoltre, è possibile ridimensionare un po 'di funzionalità delegando i test a un'altra funzione. Più concretamente:

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

Questo è più corto del mio precedente tentativo:

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

Quanto sopra è un po 'più prolisso, ma è facile da leggere IMHO e non ricalcola i confronti più volte.

Conferma visiva

In la tua risposta dici:

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

... anche, nella tua domanda, dici:

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

Forse non capisco cosa stai cercando di ottenere: vuoi copiare e incollare il pattern ovunque ne hai bisogno? Con una funzione come quella precedente, acquisisci il pattern una sola volta e controlli una volta per tutte che tutti i confronti utilizzano a , b e c come richiesto. Quindi, non devi più preoccuparti quando chiami la funzione. Certo, forse nella pratica il tuo problema è un po 'più complesso di quello che hai descritto: se sì, per favore aggiungi alcuni dettagli se possibile.

TL: DR; Il tuo codice è già corretto e "pulito".

Vedo un sacco di gente che gira intorno alla risposta, ma a tutti manca la foresta tra gli alberi. Facciamo l'intera scienza del computer e l'analisi matematica per comprendere completamente questa domanda.

Innanzitutto, notiamo che abbiamo 3 variabili, ciascuna con 3 stati: & lt ;, =, o & gt ;. Il numero totale di permutazioni è 3 ^ 3 = 27 stati, che assegnerò un numero univoco, indicato con P #, per ogni stato. Questo numero P # è un sistema di numeri fattoriali .

Enumerazione di tutte le permutazioni che abbiamo:

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

Dall'ispezione vediamo che abbiamo:

• 3 stati dove A è il massimo,
• 3 stati dove B è il massimo,
• 3 indica dove C è il massimo e
• 4 stati in cui A = B o B = C.

Scriviamo un programma (vedere la nota a piè di pagina) per enumerare tutte queste permutazioni con i valori per A, B e C. Ordinamento stabile per 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

Nel caso ti stavi chiedendo come sapevo quali stati P # erano impossibili, ora lo sai. :-)

Il numero minimo di confronti per determinare l'ordine è:

Log2 (27) = Log (27) / Log (2) = ~ 4,75 = 5 confronti

vale a dire. coredump ha dato il corretto 5 numero minimo di confronti. Vorrei formattare il suo codice come:

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

Per il tuo problema non ci interessa testare l'uguaglianza, quindi possiamo omettere 2 test.

Non importa quanto sia pulito / cattivo il codice se riceve la risposta sbagliata, quindi questo è un buon segno che stai gestendo correttamente tutti i casi!

Successivamente, per quanto riguarda la semplicità, le persone continuano a provare a "migliorare" la risposta, dove pensano che migliorare significa "ottimizzare" il numero di confronti, ma non è esattamente quello che stai chiedendo. Hai confuso tutti dove hai chiesto "Sento che potrebbe esserci un miglioramento" ma non ho definito cosa significa "meglio". Meno confronti? Meno codice? Confronti ottimali?

Ora, poiché stai chiedendo la leggibilità del codice (per correttezza), apporterei solo una modifica al tuo codice per la leggibilità: allinea il primo test con gli altri.

        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

Personalmente lo scriverei in questo modo, ma questo potrebbe essere troppo poco ortodosso per i tuoi standard di codifica:

        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

Nota a margine: questo è il codice C ++ per generare le permutazioni:

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

Modifiche: in base al feedback, spostato TL: DR in alto, rimossa tabella non ordinata, chiarito 27, ripulito il codice, descritto stati impossibili.

@msw ti ha detto di usare una matrice invece di a, b, c e @Basile ti ha detto di rifattare la logica "max" in una funzione. La combinazione di queste due idee porta a

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

quindi fornire una funzione che calcola l'indice max di un array arbitrario:

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

e chiamalo come

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

Il numero totale di LOC sembra essere aumentato rispetto a quello originale, ma ora hai la logica di base in una funzione riutilizzabile e se riesci a riutilizzare la funzione più volte, inizia a dare i suoi frutti. Inoltre, la funzione FindStrictMaxIndex non è più intessuta con i tuoi "requisiti aziendali" (separazione delle preoccupazioni), quindi il rischio che dovrai modificarlo in seguito è molto più basso rispetto alla versione originale (principio aperto-chiuso). Ad esempio, tale funzione non dovrà essere modificata anche se il numero di argomenti cambia o se è necessario utilizzare altri valori di ritorno rispetto a MOSTLY_ABC oppure se si elaborano altre variabili rispetto a a, b, c. Inoltre, l'uso di un array al posto di 3 diversi valori a, b, c potrebbe semplificare il codice anche in altri posti.

Naturalmente, se nel tuo intero programma ci sono solo uno o due posti per chiamare questa funzione, e non hai altre applicazioni per mantenere i valori in un array, allora probabilmente lascerei il codice originale così com'è (o usa il miglioramento di @ coredump).

Probabilmente dovresti usare una macro o una funzione MAX che dà il massimo di due numeri.

Quindi vuoi solo:

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

Potresti aver definito

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

ma sii cauto, in particolare sugli effetti collaterali, quando usi le macro (dato che MAX(i++,j--) si comporterebbe in modo strano)

Quindi, meglio definire una funzione

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

e usalo (o almeno #define MAX(X,Y) max2ints((X),(Y)) ....)

Se hai bisogno di capire l'origine del MAX potresti avere una macro lunga come #define COMPUTE_MAX_WITH_CAUSE(Status,Result,X,Y,Z) che è una lunga do{ ... }while(0) macro, forse

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

Quindi potresti invocare COMPUTE_MAX_WITH_CAUSE(status,res,a,b,c) in più punti. È un po 'brutto. Ho definito le variabili locali x , y , z per diminuire gli effetti collaterali negativi ....

Ho avuto più di una riflessione su questo, quindi poiché il mio problema era per lo più una conferma visiva che tutti i confronti usavano le stesse variabili, penso che questo potrebbe essere un approccio utile:

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

Ogni macro prende a , b e c nello stesso ordine è facile da confermare e il nome della macro mi consente di capire quali sono i confronti e gli AND.