Jak mogę edytować łańcuch instrukcji if-else, jeśli są zgodne z zasadami Clean Code firmy Uncle Bob?

Staram się stosować do czystego kodu wuja Boba, a konkretnie, aby metody były krótkie.

Nie mogę jednak skrócić tej logiki:

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

Nie mogę usunąć pozostałych i tym samym podzielić całej rzeczy na mniejsze części, ponieważ „else” w „else if” pomaga wydajności - ocena tych warunków jest droga i jeśli mogę uniknąć oceny poniższych warunków, spowoduję, że jeden z pierwszych to prawda, chcę ich uniknąć.

Nawet semantycznie rzecz biorąc, ocena następnego warunku, jeśli poprzednie zostało spełnione, nie ma sensu z biznesowego punktu widzenia.

edycja: To pytanie zostało zidentyfikowane jako możliwy duplikat eleganckich sposobów radzenia sobie, jeśli (jeśli inaczej) inaczej .

Uważam, że jest to inne pytanie (można to zobaczyć również poprzez porównanie odpowiedzi na te pytania).

• Moje pytanie dotyczy sprawdzenia, czy pierwszy warunek akceptacji szybko się kończy .
• Połączone pytanie stara się spełnić wszystkie warunki , aby coś zrobić. (lepiej widać w odpowiedzi na to pytanie: https://softwareengineering.stackexchange.com/a/122625/96955 )

Idealnie uważam, że powinieneś wyodrębnić swoją logikę, aby wprowadzić kod / numer alertu do własnej metody. Twój istniejący kod został zredukowany do

{
    addAlert(GetConditionCode());
}

i masz GetConditionCode () enkapsuluje logikę sprawdzania warunków. Może także lepiej użyć Enum niż magicznej liczby.

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

Ważnym pomiarem jest złożoność kodu, a nie wielkość bezwzględna. Zakładając, że różne warunki są tak naprawdę pojedynczymi wywołaniami funkcji, podobnie jak działania nie są bardziej złożone niż to, co pokazałeś, powiedziałbym, że nie ma nic złego w kodzie. To jest już tak proste, jak to tylko możliwe.

Każda próba dalszego „uproszczenia” naprawdę skomplikuje sprawę.

Oczywiście możesz zastąpić elsesłowo kluczowe na, returnjak sugerowali inni, ale to tylko kwestia stylu, a nie zmiana złożoności.

Na bok:

Moja ogólna rada brzmiałaby: nigdy nie upominaj się o żadnej zasadzie czystego kodu: większość porad dotyczących kodowania, które widzisz w Internecie, jest dobra, jeśli zastosujesz je w odpowiednim kontekście, ale radykalne zastosowanie tej samej porady wszędzie może wygrać wpis w IOCCC . Sztuka polega na tym, aby zawsze znaleźć równowagę, która pozwoli ludziom łatwo zrozumieć kod.

Używaj zbyt dużych metod, a jesteś wkręcony. Użyj zbyt małych funkcji, a będziesz wkręcony. Unikaj wyrażeń potrójnych, a jesteś przykręcony. Wszędzie używaj wyrażeń potrójnych, a ty jesteś przykręcony. Uświadom sobie, że są miejsca, które wymagają funkcji jednowierszowych i miejsca, które wymagają funkcji 50-liniowych (tak, istnieją!). Uświadom sobie, że są miejsca, które wymagają if()wyciągów i że są miejsca, które wymagają ?:operatora. Skorzystaj z pełnego arsenału, który jest do Twojej dyspozycji, i zawsze staraj się używać najlepiej dopasowanego narzędzia, jakie możesz znaleźć. I pamiętajcie, nie bądźcie religijni nawet na temat tej rady.

Kontrowersyjne jest, czy jest to „lepsze” niż zwykłe, jeśli… w danym przypadku. Ale jeśli chcesz spróbować czegoś innego, jest to powszechny sposób na zrobienie tego.

Umieść swoje warunki w obiektach i umieść te obiekty na liście

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

Jeśli pod warunkiem jest wymaganych wiele akcji, możesz wykonać szaloną rekursję

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

Z pewnością tak. Działa to tylko wtedy, gdy masz wzorzec logiki. Jeśli spróbujesz wykonać super ogólną rekursywną akcję warunkową, wówczas konfiguracja obiektu będzie tak skomplikowana jak oryginalna instrukcja if. Będziesz wymyślał swój nowy język / framework.

Ale Twój przykład nie mają wzór

Typowym przypadkiem użycia tego wzorca byłaby walidacja. Zamiast :

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

}

Staje się

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

Rozważ użycie return;po spełnieniu jednego warunku, oszczędza ci to wszystko else. Możesz nawet być w stanie return addAlert(1)bezpośrednio, jeśli ta metoda ma wartość zwracaną.

Czasami widziałem takie konstrukcje uważane za czystsze:

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

Trójskładnik z odpowiednimi odstępami może być również zgrabną alternatywą:

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

Sądzę, że możesz także spróbować utworzyć tablicę z parą zawierającą warunek i funkcję i iterować nad nim do momentu spełnienia pierwszego warunku - co, jak widzę, byłoby równe pierwszej odpowiedzi Ewana.

Jako wariant odpowiedzi @ Ewan, możesz stworzyć łańcuch (zamiast „płaskiej listy”) takich warunków:

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

W ten sposób możesz zastosować swoje warunki w określonej kolejności, a infrastruktura (pokazana klasa abstrakcyjna) pomija pozostałe kontrole po spełnieniu pierwszego.

Jest to miejsce, w którym przewyższa podejście oparte na „płaskiej liście”, gdzie musisz zaimplementować „pomijanie” w pętli, która stosuje warunki.

Po prostu konfigurujesz łańcuch warunków:

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

I rozpocznij ocenę od prostego połączenia:

c1.alertOrPropagate(display);

Po pierwsze, oryginalny kod nie jest straszny IMO. Jest to całkiem zrozumiałe i nie ma w tym nic złego.

Jeśli więc ci się nie podoba, skorzystaj z pomysłu @ Ewana, by użyć listy, ale usuń nieco nienaturalny foreach breakwzór:

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

Teraz dostosuj to w wybranym języku, spraw, aby każdy element listy stał się obiektem, krotką, cokolwiek, i jesteś dobry.

EDYCJA: wygląda na to, że nie jest tak jasne, jak myślałem, więc pozwól mi wyjaśnić dalej. conditionsjest jakąś uporządkowaną listą; headjest bieżącym badanym elementem - na początku jest to pierwszy element listy i za każdym razem, gdy next()jest wywoływany, staje się kolejnym; check()i alert()to checkConditionX()i addAlert(X)z PO.

Pytanie nie ma pewnych szczegółów. Jeśli warunki są następujące:

• może ulec zmianie lub
• powtórzone w innych częściach aplikacji lub systemu lub
• zmodyfikowane w niektórych przypadkach (takie jak różne kompilacje, testy, wdrożenia)

lub jeśli zawartość addAlertjest bardziej skomplikowana, prawdopodobnie lepszym rozwiązaniem, powiedzmy c #, byłoby:

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

Krotki nie są tak piękne w c # <8, ale zostały wybrane dla wygody.

Zalety tej metody, nawet jeśli żadna z powyższych opcji nie ma zastosowania, polega na tym, że struktura jest typowana statycznie. Nie możesz przypadkowo spieprzyć, powiedzmy, pominięcie else.

Najlepszym sposobem na zmniejszenie złożoności Cyclomatic w przypadkach, gdy jest ich dużo, if->then statementsjest użycie słownika lub listy (zależnej od języka) do przechowywania wartości klucza (jeśli wartość instrukcji lub pewnej wartości), a następnie wyniku wartość / funkcja.

Na przykład zamiast (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

Mogę po prostu

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

Jeśli używasz bardziej nowoczesnych języków, możesz zapisać więcej logiki niż tylko wartości (c #). To są naprawdę tylko funkcje wbudowane, ale możesz również wskazać inne funkcje, jeśli logika ma zawężać do wbudowania.

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

Staram się stosować do czystego kodu wuja Boba, a konkretnie, aby metody były krótkie.

Nie mogę jednak skrócić tej logiki:

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

Twój kod jest już za krótki, ale sama logika nie powinna być zmieniana. Na pierwszy rzut oka wygląda na to, że powtarzasz się z czterema wywołaniami checkCondition(), a po dokładnym przeczytaniu kodu każde z nich jest inne. Należy dodać prawidłowe formatowanie i nazwy funkcji, np .:

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

Twój kod powinien być czytelny przede wszystkim. Po przeczytaniu kilku książek wuja Boba uważam, że jest to przesłanie, które konsekwentnie próbuje przekazać.

Załóżmy, że wszystkie funkcje są zaimplementowane w tym samym komponencie, możesz sprawić, że funkcje zachowają pewien stan, aby pozbyć się wielu gałęzi w przepływie.

EG: checkCondition1()stałby się evaluateCondition1(), na którym sprawdzałby, czy spełniono poprzedni warunek; jeśli tak, to buforuje pewną wartość do odzyskania getConditionNumber().

checkCondition2()stanie się evaluateCondition2(), na którym sprawdzi, czy poprzednie warunki zostały spełnione. Jeśli poprzedni warunek nie został spełniony, sprawdza scenariusz warunku 2, buforując wartość do pobrania getConditionNumber(). I tak dalej.

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

EDYTOWAĆ:

Oto, jak należałoby wdrożyć kontrolę drogich warunków, aby to podejście zadziałało.

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

Dlatego jeśli masz zbyt wiele drogich kontroli do wykonania, a elementy w tym kodzie pozostają prywatne, takie podejście pomaga w utrzymaniu go, umożliwiając w razie potrzeby zmianę kolejności kontroli.

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

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

Ta odpowiedź stanowi tylko alternatywną sugestię z innych odpowiedzi i prawdopodobnie nie będzie lepsza niż oryginalny kod, jeśli weźmiemy pod uwagę tylko 4 wiersze kodu. Chociaż nie jest to okropne podejście (i żadne nie utrudnia konserwacji, jak powiedzieli inni), biorąc pod uwagę wspomniany scenariusz (zbyt wiele kontroli, tylko główna funkcja ujawniona jako publiczna, wszystkie funkcje są szczegółami implementacji tej samej klasy).

Więcej niż dwie klauzule „else” zmusza czytelnika kodu do przejścia całego łańcucha w celu znalezienia interesującego go kodu. Użyj metody, takiej jak: void AlertUponCondition (Warunek) {switch (warunek) {case Condition.Con1: ... break; case Condition.Con2: ... przerwa; itp ...} Gdzie „Warunek” jest właściwym wyliczeniem. W razie potrzeby zwróć wartość bool lub wartość. Nazwij to tak: AlertOnCondition (GetCondition ());

To naprawdę nie może być prostsze, I jest szybsze niż łańcuch if-else, gdy przekroczysz kilka przypadków.

Nie mogę mówić o twojej konkretnej sytuacji, ponieważ kod nie jest konkretny, ale ...

taki kod często pachnie brakującym modelem OO. Naprawdę masz cztery rodzaje rzeczy, każda związana z własnym typem alertera, ale zamiast rozpoznawać te encje i tworzyć dla nich instancję klasy, traktujesz je jako jedną rzecz i próbujesz nadrobić to później, w czasie musisz wiedzieć, z czym masz do czynienia, aby kontynuować.

Polimorfizm mógł ci bardziej odpowiadać.

Bądź podejrzliwy w stosunku do kodu za pomocą długich metod zawierających długie lub złożone konstrukcje if-then. Często potrzebujesz drzewa klasy z pewnymi wirtualnymi metodami.