Come rappresentare un albero long se else in maniera concisa

Non vorrei andare con l'idea della matrice. Mentre è meno codice, non lo vedo più leggibile. Se non altro, penso che sia più difficile da leggere perché non posso valutare mentalmente una cosa e poi ignorare la metà del codice lasciato nel metodo. Devo controllare tutto.

Vorrei iniziare semplicemente lavorando sulla riduzione del nesting. Ad esempio, guardando il blocco più esterno e il blocco else hai questo:

if (equalz(in.a, "LOC")) {
    ...
} else {
    if(varEqualsOneOf(in.a,"MNC","BAN","LCI","CTV","LEA","INS")) {
        ...
    } else {
        return Action.AC000;
    }
}

Poiché il blocco più esterno ha solo un altro nid se non altro e nessun codice comune, puoi spostarlo di un livello in modo da avere questo:

if (equalz(in.a, "LOC")) {
    ...
} else if(varEqualsOneOf(in.a,"MNC","BAN","LCI","CTV","LEA","INS")) {
    ...
} else {
    return Action.AC000;
}

Se applichiamo lo stesso principio a questo blocco:

if(varEqualsOneOf(in.e,"RRG","NL")//40
    && lessThan(in.f,in.g)) {
    return Action.AC015;
} else {
    if(varEqualsOneOf(in.e,"FLG","FLR")) {//50
        return Action.AC015;
    } else {
        return Action.AC000;
    }
}

otteniamo:

if(varEqualsOneOf(in.e,"RRG","NL")//40
    && lessThan(in.f,in.g)) {
    return Action.AC015;
} else if(varEqualsOneOf(in.e,"FLG","FLR")) {//50
    return Action.AC015;
} else {
    return Action.AC000;
}

Aspetta un minuto. Il if e il else if branch restituiscono la stessa cosa. Possiamo combinarli. Se lo facciamo, otteniamo:

if((varEqualsOneOf(in.e,"RRG","NL")/*40*/ && lessThan(in.f,in.g))
    || (varEqualsOneOf(in.e,"FLG","FLR") /*50*/)) {
    return Action.AC015;
} else {
    return Action.AC000;
}

Ora, se la situazione diventa un po 'caotica, specialmente con tutti i nomi poveri e i numeri magici. È qui che vorrei iniziare a creare funzioni di supporto che racchiudono solo le condizioni e danno loro nomi significativi. Quindi vorrei ottenere qualcosa di simile a questo:

if(IsCondition40(in) || IsCondition50(in)) {
    return Action.AC015;
} else {
    return Action.AC000;
}
.....
// obviously these still need better names but you get the idea.  Magic strings and numbers should be replaced, etc.
// also, forgive my poor java, not a language I use often enough to be any good at it
private boolean IsCondition40(In in) {
    return varEqualsOneOf(in.e,"RRG","NL") && lessThan(in.f,in.g);
}
private boolean IsCondition50(In in) {
    return varEqualsOneOf(in.e,"FLG","FLR");
}

E ora sta iniziando ad arrivare da qualche parte. Continuate a selezionarlo passo dopo passo e inizierà a migliorare. Non dimenticare di eseguire i tuoi test dopo ogni go per assicurarti di non rompere qualcosa (hai fatto test prima di iniziare, giusto?).

Cerca valori e condizioni di ritorno comuni e combinali o fai funzioni di supporto. Riduci il nidificazione. Alla fine otterrai qualcosa di molto più leggibile.

1. Cambia tutte quelle "costanti magiche" Stringhe con costanti con nome significativo. Che diamine è "FLG"?
2. Quindi modifica tutti i nomi di azioni prive di significato. "Action.AC006" non è un nome utile.
3. Cambia i commenti senza senso da "// 20", "// 30" ... (a meno che non siano in qualche modo chiari a qualcuno che lavora al progetto) Non sembrano corrispondere ai numeri ACxxx.

Solo allora prova alcuni dei refactoring menzionati nelle altre risposte. Rifiutare l'immondizia nel piombo porta alla spazzatura refactored. : -)

E un'altra cosa riguardante OOP

Oltre al valore di tmp.b , tutti delle istruzioni if coinvolgono l'oggetto in . (A meno che non me ne sia perso uno ...) Tell Do not Ask (o vedi questo SO answer ) suggerisce che il metodo decide() , tuttavia ottenga riscritto, spostato nell'oggetto in . per es.

public Action decideAction(String whateverTmpBReallyMeans);

Questo può o non può avere senso (o persino essere possibile) nel tuo progetto, ma vale la pena considerarlo.

Potrei non rispondere alla tua domanda qui, ma permettimi di dare alcuni riflessi istintivi che ho avuto mentre leggevo il codice.

Questo codice ha molte cose che lo rendono apparentemente illeggibile per me e deriva da un sacco di incomprensioni su ciò che i segmenti fanno e vengono presi di mira in modo strano. Capisco il tuo desiderio di essere in grado di seguire il flusso che ti ha portato a un certo output, ma questo sembra essere semplicemente un effetto di codice mal formattato. Se il codice è facile da seguire, non è necessaria una matrice complessa per determinare il percorso seguito per una dichiarazione di ritorno.

Prendiamolo per i principianti:

if(( //20
        equalz(tmp.b, "BA")
        && notEquals(in.c,"U")
        && equalz(in.d,"Y")
    )||(
        equalz(tmp.b, "HV")
        && notEquals(in.c,"U")
        && equalz(in.d,"Y")
        && varEqualsOneOf(in.e,"FLG","FLR","RRG"))
) {

Questo potrebbe essere contenuto all'interno di una chiamata di funzione per descrivere meglio cosa sta realmente accadendo. Qualcosa di semplice come il seguente potrebbe fare il trucco:

private boolean isFoobar(In in, Tmp tmp) {
     return (equalz(tmp.b, "BA") && notEquals(in.c,"U") && equalz(in.d,"Y"))
                || (equalz(tmp.b, "HV") && notEquals(in.c,"U") && equalz(in.d,"Y") && varEqualsOneOf(in.e,"FLG","FLR","RRG"));
}

Facendo ciò aumenta la riusabilità del controllo rendendo anche il codice più leggibile. Questo perché riduci la quantità di confusione visiva mentre, dando alla funzione un nome descrittivo, spiega al lettore ciò che stai effettivamente cercando di verificare.

Il seguente segmento potrebbe anche trarre vantaggio dall'approccio precedente una volta che è stato refactored:

if(varEqualsOneOf(in.e,"RRG","NL")//40
    && lessThan(in.f,in.g)) {
    return Action.AC015;
} else {
    if(varEqualsOneOf(in.e,"FLG","FLR")) {//50
        return Action.AC015;
    } else {
        return Action.AC000;
    }
}

Ha l'equivalente logico:

if((varEqualsOneOf(in.e,"RRG","NL") && lessThan(in.f,in.g))
        || varEqualsOneOf(in.e,"FLG","FLR")) {
    return Action.AC015;
} else {
    return Action.AC000;
}

La frase:

return Action.AC000;

È disseminato dappertutto e sembra essere il punto finale predefinito. In alcuni casi sembra ragionevole includerlo in una delle istruzioni if, ma si consideri l'utilizzo come istruzione di ritorno predefinita e la rimozione di molti dei seguenti:

else {
    return Action.AC000;
}

Se quegli articoli di Action sono significativi in pochi casi, non sarebbe possibile creare qui azioni separate che significhino anche il percorso intrapreso? Considera la seguente affermazione:

if(greaterThan(in.j,in.k)) {//110
    return Action.AC000;
}

Quale potrebbe essere:

if(greaterThan(in.j,in.k)) {//110
    return Action.AC123;
}

La mia linea di fondo è la seguente:

• Il codice che possiedi può essere notevolmente migliorato a fini di leggibilità
• Per prima cosa concentrati sui perfezionamenti della leggibilità
• Cerca di ridurre la profondità delle istruzioni if
• Prova a restituire gli elementi Action in modo univoco in modo da non dover eseguire tutto il tracciamento per le condizioni ma puoi semplicemente trovare dove è stato restituito
• La matrice di percorso dovrebbe essere utilizzata solo nel caso in cui non sia possibile creare codice pulito che è possibile seguire facendo quanto sopra

Tracciare il percorso potrebbe davvero essere la strada da percorrere nel tuo caso, perché come sempre dipende dal problema in questione, ma se fossi in te concentrerei prima i miei sforzi altrove.

P.S. Il variegato stile di formattazione utilizzato in questo segmento sta davvero attivando il mio OCD.

Un'opzione è di andare con un'implementazione di una serie di regole. In sostanza, si definisce un'interfaccia per tutti i valori che è necessario valutare. Qualcosa come:

interface Rule {
    boolean matches(Thing a, Thing b); // etc...
}

Quindi crei le istanze per ogni scenario. Questo è fondamentalmente equivalente alla soluzione di matrice che hai suggerito. L'annidamento scompare quando ogni regola è autonoma. L'ordine potrebbe essere importante e potrebbe essere un aspetto negativo.

Se questa è una buona idea dipende dal fatto che tu sappia quali sono le regole reali che sono venute a definire questo pasticcio o puoi tornare a loro. Un grande vantaggio è che questo si presta a soluzioni più dinamiche come mettere regole in un DB o creare un DSL e persino un vero motore di regole, se sei abbastanza coraggioso da intraprendere questa strada.

Stavo per rispondere alla tua domanda su Code Review, ma poi è stato chiuso lì come fuori tema, quindi ho seguito questa domanda qui. Ho lo stesso sospetto di Robzor, ovvero che il vero problema non è che l'albero if sia lungo, o che sia un albero if . Piuttosto, penso che il problema sia semplicemente che l'albero if è un pasticcio orribile - la descrizione di Robzor di essere "illeggibile" non sembra affatto un'esagerazione per me. Le altre due risposte hanno già evidenziato molti problemi con il codice, ma ne ho ancora alcuni da aggiungere, ad esempio, ci sono alcuni controlli duplicati, come all'inizio:

if((
            equalz(tmp.b, "BA")
            && notEquals(in.c,"U")
            && equalz(in.d,"Y")
        )||(
            equalz(tmp.b, "HV")
            && notEquals(in.c,"U")
            && equalz(in.d,"Y")
            && varEqualsOneOf(in.e,"FLG","FLR","RRG"))
    )

Qui, il controllo per notEquals(in.c,"U") && equalz(in.d,"Y") può essere estratto all'esterno, poiché è comune per entrambi gli argomenti OR.

Inoltre, mi chiedo che cosa notEquals(in.c,"U") restituisca quando manca in.c . Se restituisce false , quindi notEquals equivale a !equalsOrMissing , il che significa che uno dei due metodi è ridondante. E se restituisce true , quindi notEquals equivale a !equalz , quindi un metodo è ridondante in entrambi i modi.

Se hai a che fare con più di queste mostruosità inumane, penso che l'unico modo per arrivare da qualche parte con loro oltre alle fasi della disperazione e della follia è di ridimensionare gli alberi if in modo che sia effettivamente possibile guardare loro senza piangere dal puro terrore. Ho provato a farlo con il codice che hai fornito per avere un'impressione di quale logica si nasconda dietro questa facciata quasi impenetrabile del codice spaghetti, ed ecco cosa ho ottenuto:

@Override
public Action decide() {
    if (equalz(in.a, "LOC")) {
        if (notEquals(in.c, "U")
                && equalz(in.d, "Y")) {
            if (equalz(tmp.b, "BA")
                    && (varEqualsOneOf(in.e, "RRG", "NL")
                    && lessThan(in.f, in.g)
                    || varEqualsOneOf(in.e, "FLG", "FLR"))) {
                return Action.AC015;
            } else if (equalz(tmp.b, "HV")) {
                if (equalz(in.e, "RRG")) {
                    return Action.AC014;
                } else if (varEqualsOneOf(in.e, "FLG", "FLR")) {
                    return Action.AC010;
                }
            }
        } else if (!varEqualsOneOf(tmp.b, "BA", "HV")
                && equalsOrMissing(in.c, "U")
                && varEqualsOneOf(in.e, "FLG", "FLR")) {
            return Action.AC011;
        }
    } else if (varEqualsOneOf(in.a, "MNC", "BAN", "LCI", "CTV", "LEA", "INS")
            && !equalz(in.h, "A")
            && !equalz(in.i, "Y")
            && (notEquals(in.h, "U")
            && !greaterThan(in.j, in.k)
            || !notEquals(in.h, "U")
            && notEquals(in.l, "U")
            && varEqualsOneOf(in.e, "FLG", "FLR"))) {
        if (varEqualsOneOf(in.e, "FLG", "FLR", "RRG")
                || equalz(in.e, "NL")
                && equalz(in.a, "INS")) {
            if (!notEquals(in.l, "U")) {
                if (equalz(in.n, "Y")
                        || varEqualsOneOf(in.e, "FLG", "FLR")
                        && equalz(in.m, "Y")) {
                    return Action.AC012;
                } else {
                    return Action.AC002;
                }
            } else if (varEqualsOneOf(in.e, "FLG", "FLR")) {
                return Action.AC004;
            } else if (notEquals(in.o, "U")) {
                return Action.AC005;
            } else {
                return Action.AC018;
            }
        } else if (equalz(in.p, in.q)) {
            if (notEquals(in.o, "U")) {
                return Action.AC006;
            } else {
                return Action.AC008;
            }
        }
    }
    return Action.AC000;
}

Non penso che possa essere più semplice di così. Sembra ancora complicato, ma poi, l'algoritmo stesso sembra essere complicato, e non c'è modo di aggirarlo. Potrei anche aver commesso degli errori lungo la strada, quindi non fidarti che questo codice sia equivalente al tuo. Ma questa versione semplificata del tuo codice originale, che sia priva di bug o meno, potrebbe darti una base per ripensare se sia davvero la natura se un albero if non ti soddisfa, o semplicemente la struttura di quel particolare if albero il cui fortunato erede sei. E anche se decidi che vorresti ristrutturare il codice, sarà ancora più semplice per gli ordini di grandezza dopo aver districato i molti rami dell'albero if .

Considererei un database.

       {20,30,80,  
            (equalz(tmp.b, "BA")
            && notEquals(in.c,"U")
            && equalz(in.d,"Y")
            )||(
            equalz(tmp.b, "HV")
            && notEquals(in.c,"U")
            && equalz(in.d,"Y")
            && varEqualsOneOf(in.e,"FLG","FLR","RRG"))},

Non ne so abbastanza dei dati, ma sembra che quegli oggetti non cambino molto. Forse,

Action

action_id (auto-increment)
action
description


1, AC000, something
2, AC015, somethingelse

action_condition

action_condition_id (auto-increment)
condition_id
action_id
condition

1, 40, 2, RRG
2, 40, 2, NL
...

SELECT * FROM action a, action_condition ac WHERE a.action_id = ac.action_id AND ? = ?

//Build your SELECT, used Prepared Statements, 
//be careful constructing to disallow SQL Injection.

Prima che ci provassi, tuttavia, rinominerei tutto .