Come faccio a modificare una catena di if-else se le dichiarazioni aderiscono ai principi del Codice pulito di Uncle Bob?

Idealmente penso che dovresti estrarre la tua logica per ottenere il codice / numero di avviso nel suo metodo. Quindi il tuo codice esistente viene ridotto fino a

{
    addAlert(GetConditionCode());
}

e tu hai GetConditionCode () incapsulare la logica per verificare le condizioni. Forse è anche meglio usare un Enum piuttosto che un numero magico.

private AlertCode GetConditionCode() {
    if (CheckCondition1()) return AlertCode.OnFire;
    if (CheckCondition2()) return AlertCode.PlagueOfBees;
    if (CheckCondition3()) return AlertCode.Godzilla;
    if (CheckCondition4()) return AlertCode.ZombieSharkNado;
    return AlertCode.None;
}

L'importante misura è la complessità del codice, non la dimensione assoluta. Supponendo che le diverse condizioni siano in realtà solo chiamate a funzione singola, proprio come le azioni non sono più complesse di quelle che hai mostrato, direi che non c'è niente di sbagliato nel codice. È già il più semplice possibile.

Qualsiasi tentativo di "semplificare" ulteriormente complicherà le cose.

Ovviamente, puoi sostituire la parola chiave else con return come altri hanno suggerito, ma è solo una questione di stile, non un cambiamento di complessità di sorta.

A parte:

Il mio consiglio generale sarebbe, non diventare religioso su nessuna regola per il codice pulito: la maggior parte dei consigli di codifica che vedi su Internet è buona se applicata in un contesto appropriato, ma applicare radicalmente lo stesso consiglio ovunque ti possa conquistare una voce nel IOCCC . Il trucco è sempre quello di trovare un equilibrio che permetta agli esseri umani di ragionare facilmente sul tuo codice.

Usa metodi troppo grandi e sei fregato. Usa funzioni troppo piccole e sei fregato. Evita le espressioni ternarie e sei fregato. Usa le espressioni ternarie ovunque e sei fregato. Renditi conto che ci sono luoghi che richiedono funzioni a una riga e luoghi che richiedono funzioni a 50 linee (sì, esistono!). Renditi conto che ci sono luoghi che richiedono dichiarazioni if() e che ci sono posti che richiedono l'operatore ?: . Usa l'intero arsenale a tua disposizione e cerca di utilizzare sempre lo strumento più adatto che riesci a trovare. E ricorda, non diventare religioso nemmeno su questo consiglio.

È controverso se questo è "migliore" della semplice se ... sii sicuro per un dato caso. Ma se vuoi per provare qualcos'altro, questo è un modo comune per farlo.

Metti le tue condizioni negli oggetti e metti quegli oggetti in un elenco

foreach(var condition in Conditions.OrderBy(i=>i.OrderToRunIn))
{
    if(condition.EvaluatesToTrue())
    {
        addAlert(condition.Alert);
        break;
    }
}

Se sono necessarie più azioni a condizione che si possa fare qualche pazza ricorsione

void RunConditionalAction(ConditionalActionSet conditions)
{
    foreach(var condition in conditions.OrderBy(i=>i.OrderToRunIn))
    {
        if(condition.EvaluatesToTrue())
        {
            RunConditionalAction(condition);
            break;
        }
    }
}

Ovviamente si. Funziona solo se hai un modello per la tua logica. Se si tenta di creare un'azione condizionale ricorsiva super generica, l'installazione dell'oggetto sarà complicata come l'istruzione if originale. Inventerai il tuo nuovo linguaggio / framework.

Tuttavia il tuo esempio ha uno schema

Un caso di uso comune per questo modello sarebbe la convalida. Invece di:

bool IsValid()
{
    if(condition1 == false)
    {
        throw new ValidationException("condition1 is wrong!");
    }
    elseif(condition2 == false)
    {
    ....

}

diventa

[MustHaveCondition1]
[MustHaveCondition2]
public myObject()
{
    [MustMatchRegExCondition("xyz")]
    public string myProperty {get;set;}
    public bool IsValid()
    {
        conditions = getConditionsFromReflection()
        //loop through conditions
    }
}

Considera di usare return; dopo che una condizione è riuscita, ti salva tutti else s. Potresti anche riuscire a return addAlert(1) direttamente se quel metodo ha un valore di ritorno.

Ho visto a volte costruzioni come questa più pulite:

switch(true) {
    case cond1(): 
        statement1; break;
    case cond2():
        statement2; break;
    case cond3():
        statement3; break;
    // .. etc
}

Anche il ternario con spaziatura a destra può essere un'alternativa accurata:

cond1() ? statement1 :
cond2() ? statement2 :
cond3() ? statement3 : (null);

Suppongo che potresti anche provare a creare un array con una coppia contenente condizioni e funzioni e iterare su di esso fino a quando non viene soddisfatta la prima condizione - che come vedo è uguale alla prima risposta di Ewan.

Come variante della risposta di @ Ewan potresti creare una catena (invece di una "lista semplice") di condizioni come questa:

abstract class Condition {
  private static final  Condition LAST = new Condition(){
     public void alertOrPropagate(DisplayInterface display){
        // do nothing;
     }
  }
  private Condition next = Last;

  public Condition setNext(Condition next){
    this.next = next;
    return this; // fluent API
  }

  public void alertOrPropagate(DisplayInterface display){
     if(isConditionMeet()){
         display.alert(getMessage());
     } else {
       next.alertOrPropagate(display);
     }
  }
  protected abstract boolean isConditionMeet();
  protected abstract String getMessage();  
}

In questo modo puoi applicare le tue condizioni in un ordine definito e l'infrastruttura (la classe astratta mostrata) salta i rimanenti controlli dopo che il primo è stato raggiunto.

Questo è il punto in cui è superiore rispetto all'approccio "elenco semplice" in cui devi implementare il "salto" nel ciclo che applica le condizioni.

Devi semplicemente impostare la catena delle condizioni:

Condition c1 = new Condition1().setNext(
  new Condition2().setNext(
   new Condition3()
 )
);

E inizia la valutazione con una semplice chiamata:

c1.alertOrPropagate(display);

Prima di tutto, il codice originale non è terribile IMO. È piuttosto comprensibile e non c'è niente di intrinsecamente cattivo in esso.

Quindi, se non ti piace, fai tesoro dell'idea di @ Ewan di usare un elenco ma rimuovendo il suo pattern foreach break un po 'innaturale:

public class conditions
{
    private List<Condition> cList;
    private int position;

    public Condition Head
    {
        get { return cList[position];}
    }

    public bool Next()
    {
        return (position++ < cList.Count);
    }
}


while not conditions.head.check() {
  conditions.next()
}
conditions.head.alert()

Ora adatta questo nella tua lingua preferita, rendi ogni elemento della lista un oggetto, una tupla, qualunque cosa, e stai bene.

EDIT: sembra che non sia così chiaro ho pensato, quindi lasciatemi spiegare ulteriormente. conditions è un elenco ordinato di qualche tipo; head è l'elemento corrente oggetto di indagine: all'inizio è il primo elemento dell'elenco e ogni volta che viene chiamato next() diventa il seguente; check() e alert() sono checkConditionX() e addAlert(X) dall'OP.

La domanda manca di alcuni dettagli. Se le condizioni sono:

• soggetto a modifiche o
• ripetuto in altre parti dell'applicazione o del sistema o
• modificato in alcuni casi (come diverse build, test, distribuzioni)

o se il contenuto in addAlert è più complicato, allora una soluzione forse migliore in dire c # sarebbe:

//in some central spot
IEnumerable<Tuple<Func<bool>, int>> Conditions = new ... {
  Tuple.Create(CheckCondition1, 1),
  Tuple.Create(CheckCondition2, 2),
  ...
}

//at the original place
var matchingCondition = Conditions.Where(c=>c.Item1()).FirstOrDefault();
if(matchingCondition != null) 
  addAlert(matchingCondition.Item2)

Le tuple non sono così belle in c # < 8, ma scelto per convenienza.

I professionisti con questo metodo, anche se nessuna delle precedenti opzioni si applica, è che la struttura è tipizzata staticamente. Non puoi rovinare accidentalmente, diciamo, perdere un else .

Il modo migliore per ridurre La complessità ciclica nei casi in cui hai un sacco di if->then statements è usare un < em> dizionario o lista (dipendente dalla lingua) per memorizzare il valore della chiave (se il valore dell'istruzione o un valore di) e quindi un valore / risultato della funzione.

Ad esempio, anziché (C #):

if (i > 10) { return "Two"; }
else if (i > 8) { return "Four" }
else if (i > 4) { return "Eight" }
return "Ten";  //etc etc say anything after 3 or 4 values

Posso semplicemente

var results = new Dictionary<int, string>
{
  { 10, "Two" },
  { 8, "Four"},
  { 4, "Eight"},
  { 0, "Ten"},
}

foreach(var key in results.Keys)
{
  if (i > results[key]) return results.Values[key];
}

Se stai utilizzando linguaggi più moderni, puoi memorizzare più logica quindi semplicemente valori (c #). Queste sono solo funzioni inline, ma puoi anche solo puntare ad altre funzioni se la logica deve essere messa in linea.

var results = new Dictionary<Func<int, bool>, Func<int, string>>
{
  { (i) => return i > 10; ,
    (i) => return i.ToString() },
  // etc
};

foreach(var key in results.Keys)
{ 
  if (key(i)) return results.Values[key](i);
}

I'm trying to follow Uncle Bob's clean code suggestions and specifically to keep methods short.

I find myself unable to shorten this logic though:

if (checkCondition()) {addAlert(1);}
else if (checkCondition2()) {addAlert(2);}
else if (checkCondition3()) {addAlert(3);}
else if (checkCondition4()) {addAlert(4);}

Il tuo codice è già troppo corto, ma la logica stessa non dovrebbe essere modificata. A prima vista sembra che tu stia ripetendo te stesso con quattro chiamate a checkCondition() , ed è solo apparente che ognuna è diversa dopo aver riletto attentamente il codice. Dovresti aggiungere nomi di formattazione e funzione appropriati, ad esempio:

if (is_an_apple()) {
  addAlert(1);
}
else if (is_a_banana()) {
  addAlert(2);
}
else if (is_a_cat()) {
  addAlert(3);
}
else if (is_a_dog()) {
  addAlert(4);
}

Il tuo codice dovrebbe essere leggibile sopra ogni altra cosa. Dopo aver letto diversi libri dello zio Bob, credo che sia il messaggio che sta costantemente cercando di trasmettere.

Supponendo che tutte le funzioni siano implementate nello stesso componente, è possibile che le funzioni mantengano uno stato per eliminare più rami nel flusso.

EG: checkCondition1() diventerebbe evaluateCondition1() , su cui verificherebbe se la condizione precedente fosse soddisfatta; in tal caso, memorizza nella cache un valore da recuperare da getConditionNumber() .

checkCondition2() diventerebbe evaluateCondition2() , su cui verificherebbe se le condizioni precedenti fossero soddisfatte. Se la condizione precedente non è stata soddisfatta, verifica lo scenario 2 delle condizioni, memorizzando nella cache un valore che deve essere recuperato da getConditionNumber() . E così via.

clearConditions();
evaluateCondition1();
evaluateCondition2();
evaluateCondition3();
evaluateCondition4();
if (anyCondition()) { addAlert(getConditionNumber()); }

Modifica

Ecco come dovrebbe essere implementato il controllo di condizioni costose per il funzionamento di questo approccio.

bool evaluateCondition34() {
    if (!anyCondition() && A && B && C) {
        conditionNumber = 5693;
        return true;
    }
    return false;
}

...

bool evaluateCondition76() {
    if (!anyCondition() && !B && C && D) {
        conditionNumber = 7658;
        return true;
    }
    return false;
}

Pertanto, se si eseguono troppi controlli costosi e le cose in questo codice rimangono private, questo approccio aiuta a mantenerlo, consentendo di modificare l'ordine degli assegni, se necessario.

clearConditions();
evaluateCondition10();
evaluateCondition9();
evaluateCondition8();
evaluateCondition7();
...
evaluateCondition34();
...
evaluateCondition76();

if (anyCondition()) { addAlert(getConditionNumber()); }

Questa risposta fornisce solo un suggerimento alternativo dalle altre risposte e probabilmente non sarà migliore del codice originale se consideriamo solo 4 linee di codice. Anche se, questo non è un approccio terribile (e non rende la manutenzione più difficile come altri hanno detto) dato lo scenario che ho citato (troppi controlli, solo la funzione principale esposta come pubblica, tutte le funzioni sono dettagli di implementazione dello stesso classe).

Qualsiasi più di due clausole "else" obbliga il lettore del codice a passare attraverso l'intera catena per trovare quello di interesse. Usa un metodo come: void AlertUponCondition (Condizione condizione) {   interruttore (condizione)   {     caso Condition.Con1:       ...       rompere;     caso Condition.Con2:       ...       rompere;     eccetera...   } Dove "Condizione" è un enum appropriato. Se necessario, restituisci un valore o un valore booleano. Chiamalo così:   AlertOnCondition (GetCondition ());

In realtà non può essere più semplice, E una volta è più veloce della catena if-else superi alcuni casi.

Non posso parlare per la tua situazione particolare perché il codice non è specifico, ma ...

codice come quello è spesso un odore per un modello OO mancante. Hai davvero quattro tipi di cose, ognuna associata al proprio tipo di segnalatore, ma piuttosto che riconoscere queste entità e creare un'istanza di classe per ciascuna, trattale come una cosa e prova a rimediare più tardi, in un momento in cui davvero devi sapere con cosa hai a che fare per procedere.

Il polimorfismo potrebbe esserti più adatto.

Siate sospettosi del codice con lunghi metodi contenenti costrutti if-then lunghi o complessi. Spesso desideri un albero delle classi con alcuni metodi virtuali.