2009-12-18 8 views
7

A volte le condizioni possono diventare piuttosto complesse, quindi per motivi di leggibilità solitamente le divido e attribuisco a ciascun componente un nome significativo. Ciò sconfigge tuttavia la valutazione del cortocircuito, che può rappresentare un problema. Mi è venuto in mente un approccio wrapper, ma a mio avviso è troppo prolisso.Come dividere le condizioni complesse e mantenere la valutazione del cortocircuito?

Qualcuno può venire con una soluzione pulita per questo?

Vedi codice qui sotto per gli esempi di ciò che intendo:

public class BooleanEvaluator { 

    // problem: complex boolean expression, hard to read 
    public static void main1(String[] args) { 

     if (args != null && args.length == 2 && !args[0].equals(args[1])) { 
      System.out.println("Args are ok"); 
     } 
    } 

    // solution: simplified by splitting up and using meaningful names 
    // problem: no short circuit evaluation 
    public static void main2(String[] args) { 

     boolean argsNotNull = args != null; 
     boolean argsLengthOk = args.length == 2; 
     boolean argsAreNotEqual = !args[0].equals(args[1]); 

     if (argsNotNull && argsLengthOk && argsAreNotEqual) { 
      System.out.println("Args are ok"); 
     } 
    } 

    // solution: wrappers to delay the evaluation 
    // problem: verbose 
    public static void main3(final String[] args) { 

     abstract class BooleanExpression { 
      abstract boolean eval(); 
     } 

     BooleanExpression argsNotNull = new BooleanExpression() { 
      boolean eval() { 
       return args != null; 
      } 
     }; 

     BooleanExpression argsLengthIsOk = new BooleanExpression() { 
      boolean eval() { 
       return args.length == 2; 
      } 
     }; 

     BooleanExpression argsAreNotEqual = new BooleanExpression() { 
      boolean eval() { 
       return !args[0].equals(args[1]); 
      } 
     }; 

     if (argsNotNull.eval() && argsLengthIsOk.eval() && argsAreNotEqual.eval()) { 
      System.out.println("Args are ok"); 
     } 
    } 
} 

Risposta alle risposte:

Grazie per tutte le vostre idee! I seguenti alternative sono state presentate finora:

  • linee di interruzione e aggiungere commenti
  • lasciare così com'è
  • metodi Estratto
  • rendimenti primi
  • nidificati/Split in su se c'è

Interrompe le linee e aggiungi commenti:

L'aggiunta di interruzioni di riga all'interno di una condizione viene annullata dal programma di formattazione del codice in Eclipse (ctrl + shift + f). I commenti in linea aiutano con questo, ma lascia poco spazio su ogni riga e può risultare in un involucro brutto. In casi semplici questo potrebbe essere abbastanza tuttavia.

lasciare così com'è:

La condizione esempio che ho dato è piuttosto semplicistico, in modo da non avere bisogno di affrontare le questioni di leggibilità in questo caso. Stavo pensando di situazioni in cui la condizione è molto più complesso, ad esempio:

private boolean targetFound(String target, List<String> items, 
     int position, int min, int max) { 

    return ((position >= min && position < max && ((position % 2 == 0 && items 
      .get(position).equals(target)) || (position % 2 == 1) 
      && position > min && items.get(position - 1).equals(target))) 
      || (position < min && items.get(0).equals(target)) || (position >= max && items 
      .get(items.size() - 1).equals(target))); 
} 

non mi consiglia di lasciare questo così com'è.

metodi

Estratto:

ho considerato l'estrazione di metodi, come è stato suggerito in varie risposte. Lo svantaggio di questo è che questi metodi hanno in genere molto bassa granularità e non possono essere molto significativo di per sé, in modo che possa ingombrare la tua classe, per esempio:

private static boolean lengthOK(String[] args) { 
    return args.length == 2; 
} 

Questo non sarebbe davvero merita di essere un metodo separato a livello di classe. Inoltre devi passare tutti gli argomenti rilevanti per ciascun metodo. Se crei una classe separata puramente per valutare una condizione molto complessa, allora questa potrebbe essere una soluzione OK IMO.

Quello che ho cercato di ottenere con l'approccio BooleanExpression è che la logica rimane locale. Si noti che anche la dichiarazione di BooleanExpression è locale (non credo di aver mai trovato un caso d'uso per una dichiarazione di classe locale prima!).

ritorni presto:

La rapida soluzione rendimenti sembra adeguata, anche se io non favoriscono l'idioma. Una notazione alternativa:

public static boolean areArgsOk(String[] args) { 

    check_args: { 
     if (args == null) { 
      break check_args; 
     } 
     if (args.length != 2) { 
      break check_args; 
     } 
     if (args[0].equals(args[1])) { 
      break check_args; 
     } 
     return true; 
    } 
    return false; 
} 

mi rendo conto che la maggior parte delle persone odiano le etichette e le pause, e questo stile potrebbe essere troppo raro per essere considerato leggibile.

nidificati/contempla se è:

Permette l'introduzione di nomi significativi in ​​combinazione con la valutazione ottimizzata. Un inconveniente è l'albero complesso di istruzioni condizionali che possono derivarne

Showdown

Quindi, per vedere quale approccio mi utlimately prediligo, ho applicato alcune delle soluzioni proposte per l'esempio targetFound complesso di cui sopra. Qui ci sono i miei risultati:

nidificato/Spalato, se lo è, con nomi significativi molto prolisso, nomi significativi in ​​realtà non aiutano la leggibilità qui

private boolean targetFound1(String target, List<String> items, 
     int position, int min, int max) { 

    boolean result; 
    boolean inWindow = position >= min && position < max; 
    if (inWindow) { 

     boolean foundInEvenPosition = position % 2 == 0 
       && items.get(position).equals(target); 
     if (foundInEvenPosition) { 
      result = true; 
     } else { 
      boolean foundInOddPosition = (position % 2 == 1) 
        && position > min 
        && items.get(position - 1).equals(target); 
      result = foundInOddPosition; 
     } 
    } else { 
     boolean beforeWindow = position < min; 
     if (beforeWindow) { 

      boolean matchesFirstItem = items.get(0).equals(target); 
      result = matchesFirstItem; 
     } else { 

      boolean afterWindow = position >= max; 
      if (afterWindow) { 

       boolean matchesLastItem = items.get(items.size() - 1) 
         .equals(target); 
       result = matchesLastItem; 
      } else { 
       result = false; 
      } 
     } 
    } 
    return result; 
} 

nidificato/split se di, con i commenti meno prolissa, ma ancora difficile da leggere e facile da creare bug

private boolean targetFound2(String target, List<String> items, 
     int position, int min, int max) { 

    boolean result; 
    if ((position >= min && position < max)) { // in window 

     if ((position % 2 == 0 && items.get(position).equals(target))) { 
      // even position 
      result = true; 
     } else { // odd position 
      result = ((position % 2 == 1) && position > min && items.get(
        position - 1).equals(target)); 
     } 
    } else if ((position < min)) { // before window 
     result = items.get(0).equals(target); 
    } else if ((position >= max)) { // after window 
     result = items.get(items.size() - 1).equals(target); 
    } else { 
     result = false; 
    } 
    return result; 
} 

torna presto ancora più compatto, ma l'albero condizionale rimane altrettanto complesso

private boolean targetFound3(String target, List<String> items, 
     int position, int min, int max) { 

    if ((position >= min && position < max)) { // in window 

     if ((position % 2 == 0 && items.get(position).equals(target))) { 
      return true; // even position 
     } else { 
      return (position % 2 == 1) && position > min && items.get(
        position - 1).equals(target); // odd position 
     } 
    } else if ((position < min)) { // before window 
     return items.get(0).equals(target); 
    } else if ((position >= max)) { // after window 
     return items.get(items.size() - 1).equals(target); 
    } else { 
     return false; 
    } 
} 

metodi estratti risultati in metodi privi di senso nella tua classe il passaggio di parametri è fastidioso

private boolean targetFound4(String target, List<String> items, 
     int position, int min, int max) { 

    return (foundInWindow(target, items, position, min, max) 
      || foundBefore(target, items, position, min) || foundAfter(
      target, items, position, max)); 
} 

private boolean foundAfter(String target, List<String> items, int position, 
     int max) { 
    return (position >= max && items.get(items.size() - 1).equals(target)); 
} 

private boolean foundBefore(String target, List<String> items, 
     int position, int min) { 
    return (position < min && items.get(0).equals(target)); 
} 

private boolean foundInWindow(String target, List<String> items, 
     int position, int min, int max) { 
    return (position >= min && position < max && ((position % 2 == 0 && items 
      .get(position).equals(target)) || (position % 2 == 1) 
      && position > min && items.get(position - 1).equals(target))); 
} 

espressione booleana wrapper rivisitato nota che i parametri del metodo devono essere dichiarati definitivi per questo caso complesso il livello di dettaglio è difendibile IMO Forse chiusure renderà questo più facile, se mai d'accordo su quello (-

private boolean targetFound5(final String target, final List<String> items, 
     final int position, final int min, final int max) { 

    abstract class BooleanExpression { 
     abstract boolean eval(); 
    } 

    BooleanExpression foundInWindow = new BooleanExpression() { 

     boolean eval() { 
      return position >= min && position < max 
        && (foundAtEvenPosition() || foundAtOddPosition()); 
     } 

     private boolean foundAtEvenPosition() { 
      return position % 2 == 0 && items.get(position).equals(target); 
     } 

     private boolean foundAtOddPosition() { 
      return position % 2 == 1 && position > min 
        && items.get(position - 1).equals(target); 
     } 
    }; 

    BooleanExpression foundBefore = new BooleanExpression() { 
     boolean eval() { 
      return position < min && items.get(0).equals(target); 
     } 
    }; 

    BooleanExpression foundAfter = new BooleanExpression() { 
     boolean eval() { 
      return position >= max 
        && items.get(items.size() - 1).equals(target); 
     } 
    }; 

    return foundInWindow.eval() || foundBefore.eval() || foundAfter.eval(); 
} 

Immagino che dipende molto dalla situazione (come sempre). Per condizioni molto complesse l'approccio del wrapper potrebbe essere difendibile, sebbene non sia comune.

Grazie per tutto il vostro contributo!

MODIFICA: ripensamento. Potrebbe essere ancora migliore per creare una classe specifica per la logica complessa come questa:

import java.util.ArrayList; 
import java.util.List; 

public class IsTargetFoundExpression { 

    private final String target; 
    private final List<String> items; 
    private final int position; 
    private final int min; 
    private final int max; 

    public IsTargetFoundExpression(String target, List<String> items, int position, int min, int max) { 
     this.target = target; 
     this.items = new ArrayList(items); 
     this.position = position; 
     this.min = min; 
     this.max = max; 
    } 

    public boolean evaluate() { 
     return foundInWindow() || foundBefore() || foundAfter(); 
    } 

    private boolean foundInWindow() { 
     return position >= min && position < max && (foundAtEvenPosition() || foundAtOddPosition()); 
    } 

    private boolean foundAtEvenPosition() { 
     return position % 2 == 0 && items.get(position).equals(target); 
    } 

    private boolean foundAtOddPosition() { 
     return position % 2 == 1 && position > min && items.get(position - 1).equals(target); 
    } 

    private boolean foundBefore() { 
     return position < min && items.get(0).equals(target); 
    } 

    private boolean foundAfter() { 
     return position >= max && items.get(items.size() - 1).equals(target); 
    } 
} 

La logica è sufficiente a giustificare una classe separata complessa (e test di unità, yay!).Manterrà il codice utilizzando questa logica più leggibile e promuoverà il riutilizzo nel caso in cui questa logica sia necessaria altrove. Penso che questa sia una bella lezione perché ha una sola responsabilità e solo campi finali.

+0

Mi piace la tua prima soluzione, tuttavia con un'appropriata denominazione delle variabili, potrebbe non essere necessario. La tua seconda soluzione è troppo verbosa IMO. – BacMan

risposta

6

trovo a capo e gli spazi che faccio un buon lavoro, in realtà:

public static void main1(String[] args) { 

    if (args != null 
     && args.length == 2 
     && !args[0].equals(args[1]) 
     ) { 
      System.out.println("Args are ok"); 
    } 
} 

Bisogna ammettere che funziona meglio con il mio (impopolare) rinforzi stile (non mostrato sopra), ma anche con la sopra di esso funziona bene se si mette il paren di chiusura e il tutore di apertura sulla propria linea (quindi non vengono persi alla fine dell'ultima condizione).

io a volte anche commentare i singoli bit:

public static void main1(String[] args) { 

    if (args != null    // Must have args 
     && args.length == 2   // Two of them, to be precise 
     && !args[0].equals(args[1]) // And they can't be the same 
     ) { 
      System.out.println("Args are ok"); 
    } 
} 

Se davvero si vuole chiamare le cose, più if s lo farà:

public static void main1(String[] args) { 

    if (args != null) { 
     if (args.length == 2) { 
      if (!args[0].equals(args[1])) { 
       System.out.println("Args are ok"); 
      } 
     } 
    } 
} 

... e qualsiasi compilatore ottimizzato lo faranno collassare quello. Per me, probabilmente è un po 'troppo prolisso, però.

+0

Multiple se è la soluzione più naturale e più chiara. Mostra esattamente cosa sta succedendo. Puoi anche aggiungere commenti su ogni riga. Ma perché avresti bisogno di ottimizzare il compilatore? Non c'è niente da collassare. – PauliL

+0

I commenti su ogni riga hanno il vantaggio aggiunto del formattatore Eclipse di non modificare le interruzioni di riga. –

+0

Nella maggior parte dei casi, la nidificazione profonda è considerata una cattiva pratica. Tuttavia non sono sicuro che questo caso particolare sia un'eccezione. Tuttavia, in effetti, migliora la leggibilità. – BalusC

6

Se il tuo obiettivo è la leggibilità, perché non interrompere semplicemente le linee e aggiungere commenti?

if (args != null    // args not null 
     && args.length == 2   // args length is OK 
     && !args[0].equals(args[1]) // args are not equal 
    ) { 

     System.out.println("Args are ok"); 
    } 
+0

Hai cambiato la sua logica lì ... –

+0

Il mio male. Risolto :-) –

+0

Ho corretto il bit rimanente –

11

È possibile utilizzare i rendimenti prime (da un metodo) per ottenere lo stesso effetto:

[Alcune correzioni applicate]

public static boolean areArgsOk(String[] args) { 
    if(args == null) 
     return false; 

    if(args.length != 2) 
     return false; 

    if(args[0].equals(args[1])) 
     return false; 

    return true; 
    } 

    public static void main2(String[] args) { 

     boolean b = areArgsOk(args); 
     if(b) 
      System.out.println("Args are ok"); 
    } 
+0

Hai dimenticato di passare in args a areArgsOk(). – BalusC

+0

Hai ragione. Grazie. Fisso. –

+0

Attenzione! Ha && - condizioni. Utilizzare if (args == null) return false; if (args.length! = 2) restituisce false; if (args [0] .equals (args [1])) restituisce false; return true; – r3zn1k

0

Split fuori in una funzione separata (per migliorare la leggibilità in main()) e aggiungere un commento (così la gente a capire che cosa si sta cercando di realizzare)

public static void main(String[] args) { 
    if (argumentsAreValid(args)) { 
      System.out.println("Args are ok"); 
    } 
} 


public static boolean argumentsAreValid(String[] args) { 
    // Must have 2 arguments (and the second can't be the same as the first) 
    return args == null || args.length == 2 || !args[0].equals(args[1]); 
} 

ETA: Mi piace anche l'idea di Itay di utilizzare i ritorni anticipati nella funzione ArgumentsAreValid per migliorare la leggibilità.

+0

Grazie per la risposta. Non vedo davvero come l'estrazione di un metodo in questo modo migliori le cose. L'aggiunta di un commento è utile. Oh sì, "bool" dovrebbe essere booleano ei nomi dei metodi iniziano con lettere minuscole in Java. –

+0

@Adriaan - ha risolto il codice lì - diventando troppo familiare con C# ora e utilizzando il mio Java. Personalmente, penso che dividerlo lo renda più leggibile, ma questo è certamente un giudizio soggettivo. –

-1

Non c'è davvero niente di sbagliato nel primo pezzo di codice; Stai pensando troppo a cose, IMO.

Ecco un altro modo che, se dettagliato, è facile da capire.

static void usage() { 
    System.err.println("Usage: blah blah blah blah"); 
    System.exit(-1); 
} 

// ... 

if (args == null || args.length < 2) 
    usage(); 
if (args[0].equals(args[1])) 
    usage() 
+0

Anche se non fa esattamente la stessa cosa in presenza di un gestore della sicurezza. –

+0

Il mio esempio è stato semplice per brevità. Consideri una condizione veramente complessa Il suddetto diviso in due dichiarazioni condizionali mi sembra arbitrario. –

+0

Sì, certo che è arbitrario! Scrivi codice per le persone da leggere, così puoi esprimere giudizi su ciò che è più chiaro. –

2

Il tipo di espressione BooleanExpression non sembra essere abbastanza utile per essere una classe al di fuori della classe principale e aggiunge un po 'di peso intellettuale alla vostra applicazione. Vorrei semplicemente scrivere metodi privati ​​che sono nominati in modo appropriato per eseguire i controlli desiderati. È molto più semplice

+0

Penso che la creazione di metodi privati ​​sia ancora troppo invadente (e richiede il passaggio di molti parametri). Espressione booleana rientra nell'ambito del metodo, quindi posso suddividere la mia condizione senza ingombrare la mia classe di alto livello. –

+0

Non sono d'accordo. L'utilizzo di metodi privati ​​è molto più facile da capire per chiunque piuttosto che incorporare una classe in un metodo. In realtà, il modo in cui BooleanExpression è definito, si potrebbe anche usare solo metodi privati. Non solo, ma poiché sono privati, possono essere modificati in seguito. Se questo fosse un linguaggio come Groovy o Python, allora direi passare un metodo (o una chiusura) per gestire la validazione. Dal momento che non lo è, è meglio mantenere le cose semplici (le persone che devono supportarlo in futuro ti ringrazieranno per questo). L'espressione booleana è un tentativo di imitare le chiusure (o i parametri del metodo). –

2

È necessario eseguire l'assegnazione variabile INSIDE if.

if (a && b && c) ... 

traduce

calculatedA = a; 
if (calculatedA) { 
    calculatedB = b; 
    if (calculatedB) { 
    calculatedC = c; 
    if (calculatedC) .... 
    } 
} 

Spesso è utile per fare questo in ogni caso dal momento che i nomi dei concetti che si stanno testando perché, come si dimostra chiaramente nel codice esempio. Questo aumenta la leggibilità.

1

La tua prima soluzione è buona per questo tipo di complessità. Se le condizioni fossero più complesse, scriverei metodi privati ​​per ogni controllo che è necessario eseguire.Qualcosa di simile:

public class DemoStackOverflow { 

    public static void main(String[] args) { 
    if (areValid(args)) { 
     System.out.println("Arguments OK"); 
    } 
    } 

    /** 
    * Validation of parameters. 
    * 
    * @param args an array with the parameters to validate. 
    * @return true if the arguments are not null, the quantity of arguments match 
    * the expected quantity and the first and second are not equal; 
    *   false, otherwise. 
    */ 
    private static boolean areValid(String[] args) { 
     return notNull(args) && lengthOK(args) && areDifferent(args); 
    } 

    private static boolean notNull(String[] args) { 
     return args != null; 
    } 

    private static boolean lengthOK(String[] args) { 
     return args.length == EXPECTED_ARGS; 
    } 

    private static boolean areDifferent(String[] args) { 
     return !args[0].equals(args[1]); 
    } 

    /** Quantity of expected arguments */ 
    private static final int EXPECTED_ARGS = 2; 

} 
+0

Questa è una illustrazione della mia risposta suggerita. +1 –

0

Alcune buone soluzioni già, ma ecco la mia variazione. Di solito faccio il controllo null come la clausola di guardia più in alto in tutti i metodi (rilevanti). Se lo faccio in questo caso, lascia solo il controllo di lunghezza e uguaglianza nel successivo se, che potrebbe già essere considerato una sufficiente riduzione di complessità e/o miglioramento della leggibilità?

public static void main1(String[] args) { 

     if (args == null) return; 

     if (args.length == 2 && !args[0].equals(args[1])) { 
      System.out.println("Args are ok"); 
     } 
    } 
+0

Pragmatico, ma non sufficiente per il caso complesso che penso. –

0

La tua prima soluzione non funzionerà in molti casi, incluso l'esempio che fornisci sopra. Se args è nullo, quindi

boolean argsNotNull = args != null; 
// argsNotNull==false, okay 
boolean argsLengthOk = args.length == 2; 
// blam! null pointer exception 

Un vantaggio del cortocircuito è che si risparmia in runtime. Un altro vantaggio è che consente di eseguire test precoci che verificano le condizioni che comporterebbero test successivi generando eccezioni.

Personalmente, quando i test sono individualmente semplici e tutto ciò che rende complesso è che ce ne sono molti, voterei per la semplice soluzione "aggiungi alcune interruzioni di linea e commenti". Questo è più facile da leggere rispetto alla creazione di una serie di funzioni aggiuntive.

L'unica volta che suddivido le cose in subroutine è quando un singolo test è complesso. Se avete bisogno di uscire e di leggere un database o eseguire un grosso calcolo, poi certo, roll che in una subroutine in modo che il codice di livello superiore è un facile da leggere

if (salesType=='A' && isValidCustomerForSalesTypeA(customerid)) 
etc 

Edit: come mi piacerebbe rompere l'esempio più complesso che hai dato.

Quando ottengo davvero condizioni complesse, cerco di suddividerle in IF nidificati per renderle più leggibili. Come ... e mi scusi se il seguente non è davvero equivalente al tuo esempio, non volevo studiare le parentesi troppo da vicino solo per un esempio come questo (e ovviamente le parentesi sono ciò che rende difficile leggere) :

if (position < min) 
{ 
    return (items.get(0).equals(target)); 
} 
else if (position >= max) 
{ 
    return (items.get(items.size() - 1).equals(target)); 
} 
else // position >=min && < max 
{ 
    if (position % 2 == 0) 
    { 
    return items.get(position).equals(target); 
    } 
    else // position % 2 == 1 
    { 
    return position > min && items.get(position - 1).equals(target); 
    } 
} 

Mi sembra leggibile perché è probabile che venga visualizzato. In questo esempio, la condizione di "primo livello" è chiaramente il rapporto tra la posizione in min e max, quindi rompere questo aiuta a chiarire le cose, secondo me.

In realtà, quanto sopra è probabilmente più efficiente rispetto a racchiuderlo tutto su una riga, perché il resto consente di ridurre il numero di confronti.

Personalmente faccio fatica quando è una buona idea mettere una condizione complessa in una singola riga. Ma anche se lo capisci, le probabilità sono che la prossima persona che arriva non lo farà. Anche alcune cose abbastanza semplici, come

return s == null? -1: s.length();

io a volte mi dico, sì, ho capito, ma gli altri non, forse meglio scrivere

if (s==null) 
    return -1; 
    else 
    return s.length(); 
+0

La prima soluzione interrompe il cortocircuito, questo è stato il mio punto di vista. Non sto prendendo in considerazione grandi calcoli o lettura da un database qui, solo una condizione complessa dato che tutti i dati sono presenti. –

+0

@Adriaan: Ok. Avevo l'impressione che tu pensassi che fosse solo una cosa di efficienza, e non una cosa "non funziona". Bene, penso che la mia risposta sia corretta, anche se lo sapevi già! – Jay

+0

Ok, quindi come presenteresti la condizione di esempio più complessa che ho aggiunto in alto? –

1

Questa domanda è a partire dal 2009, ma in futuro (Java 8) saremo in grado di usa le espressioni Lambda che possono essere per questo contesto proprio come le espressioni booleane, ma puoi usarle in modo che vengano valutate solo quando necessario.

public static void main2(String[] args) { 

    Callable<Boolean> argsNotNull =() -> args != null; 
    Callable<Boolean> argsLengthOk =() -> args.length == 2; 
    Callable<Boolean> argsAreNotEqual =() -> !args[0].equals(args[1]); 

    if (argsNotNull.call() && argsLengthOk.call() && argsAreNotEqual.call()) { 
     System.out.println("Args are ok"); 
    } 
} 

Si può fare lo stesso con java 5/6 ma è meno efficiente e molto più economico scrivere con classi anonime.

Problemi correlati