Troppe istruzioni if-else, un modo per refactarlo

Question

Può essere refactored? o questo sembra a posto. (i nomi delle variabili sono cambiati)

if (cmpScope.equals(GLOBAL)) { 
      return true; 
     } else if ((cmpScope.equals(X) || cmpScope.equals(Y)) 
       && cid == pid) { 
      return true; 
     } else if (cmpScope.equals(Z) && cid != pId) { 
      return true; 
     } else if (cmpScope.equals(V) && cid == pid) { 
      return true; 
     } else if (cmpScope.equals(Z) && cid == pid && cSubId != pSubId) { 
      return true; 
     } 
     return false; 

Answer 1

Basta combinare tutte le espressioni con gli operatori, poiché tutti restituiscono true.

return ((cmpScope.equals(GLOBAL) || 
     ((cmpScope.equals(X) || cmpScope.equals(Y)) && cid == pid) || 
     (cmpScope.equals(Z) && cid != pId) || 
     (cmpScope.equals(V) && cid == pid) || 
     (cmpScope.equals(Z) && cid == pid && cSubId != pSubId)); 

Answer 2

Suppongo che il 'ritorno vero' sia solo un esempio? In caso contrario, è possibile raggruppare tutte le istruzioni che restituiscono true in un'espressione. Se in realtà sono destinati a fare cose diverse, per me va bene. (In alcuni casi è possibile utilizzare un'istruzione switch, non in questo caso, ma in generale potrebbe essere utile sapere che è possibile utilizzare questo https://docs.oracle.com/javase/tutorial/java/nutsandbolts/switch.html)

Answer 3

È possibile memorizzare il risultato delle condizioni nelle variabili e gestire tutte le condizioni in una dichiarazione. (Anche se io non credo che sia una soluzione migliore in tutti i casi)

boolean trueCond1 = cmpScope.equals(GLOBAL); 
boolean trueCond2 = (cmpScope.equals(X) || cmpScope.equals(Y)) && cid == pid; 
boolean trueCond3 = cmpScope.equals(Z) && cid != pId; 
boolean trueCond4 = cmpScope.equals(V) && cid == pid; 
boolean trueCond5 = cmpScope.equals(Z) && cid == pid && cSubId != pSubId; 

return (trueCond1 || trueCond2 || trueCond3 || trueCond4 || trueCond5); 

Answer 4

Si può sbarazzarsi di tutte le else s perché ogni clausola if restituisce un valore.

if (cmpScope.equals(GLOBAL)) { 
    return true; 
} 
if ((cmpScope.equals(X) || cmpScope.equals(Y)) && cid == pid) { 
    return true; 
} 
if (cmpScope.equals(Z) && cid != pId) { 
    return true; 
} 
if (cmpScope.equals(V) && cid == pid) { 
    return true; 
} 
if (cmpScope.equals(Z) && cid == pid && cSubId != pSubId) { 
    return true; 
} 
return false; 

Questa è una delle migliori tecniche per ridurre l'apparente complessità del codice. A seconda delle esigenze specifiche, il raggruppamento in una singola espressione con o operatori potrebbe essere una soluzione migliore. Anche il confronto cid/pid è comune alle ultime quattro prove, in modo da qualcosa come questo potrebbe essere ancora meglio:

if (cmpScope.equals(GLOBAL)) { 
    return true; 
} 
if (cid == pid) { 
    if (cmpScope.equals(X) || cmpScope.equals(Y)) { 
     return true; 
    } 
    if (cmpScope.equals(V)) { 
     return true; 
    } 
    if (cmpScope.equals(Z) && cSubId != pSubId) { 
     return true; 
    } 
} else { 
    if (cmpScope.equals(Z)) { 
     return true; 
    } 
} 
return false; 

Answer 5

Appoggio pienamente @ risposta Carl-manaster, ma penso che possiamo andare oltre.

Il mio approccio si basa sui seguenti principi:

• non ci dovrebbero essere else.
• le espressioni logiche complesse devono essere estratte in metodi privati ​​per aumentare la leggibilità.
• c'è una suite di test che copre tutti i rami possibili. Questo è il motivo per cui le persone sono preoccupate per la complessità ciclomatica. E questo è il motivo per cui l'estrazione nel metodo aiuta.

Dal codice di esempio, non è chiaro che cosa esattamente questo metodo, quindi ho scelto alcuni nomi casuali e l'autore dovrebbe cambiarlo in qualcosa di più valido.

Non avevo prove e non c'è tempo, quindi non sono sicuro al 100% che funzionerà, ma dopo una serie di modifiche che ho finito con seguente codice:

function(cid, pid, cSubId, pSubId) { 
    return cmpScope.equals(GLOBAL) && isValidSomething() && isValidSomethingElse(); 
} 

Dove metodi estratti sono:

function isValidSomething() { 
    if(cid != pid) { 
     return false; 
    } 
    if (cmpScope.equals(V) || cmpScope.equals(X) || cmpScope.equals(Y)) { 
     return true; 
    } 
    return cmpScope.equals(Z) && cSubId != pSubId; 
} 

function isValidSomethingElse() { 
    return cmpScope.equals(Z) && cid != pId; 
} 

È solo un esempio e puoi migliorarlo ancora di più.

Ancora una volta, il punto principale di questo è quello di avere metodi privati ​​con un buon nome in modo da poterlo leggere "come una prosa ben scritta". Immagina:

function isMyCarValid() { 
    return isHaveWheels() && isHaveFuel() && isHaveDriver(); 
}