Najlepsza praktyka dotycząca if / return

60

Chcę wiedzieć, co jest uważane za lepszy sposób zwrotu, gdy mam ifoświadczenie.

Przykład 1:

public bool MyFunction()
{
   // Get some string for this example
   string myString = GetString();

   if (myString == null)
   {
      return false;
   }
   else
   {
      myString = "Name " + myString;
      // Do something more here...
      return true;
   }
}

Przykład 2:

public bool MyFunction()
{
   // Get some string for this example
   string myString = GetString();

   if (myString == null)
   {
      return false;
   }

   myString = "Name " + myString;
   // Do something more here...
   return true;
}

Jak widać w obu przykładach, funkcja zwróci, true/falseale czy warto umieścić elseinstrukcję jak w pierwszym przykładzie, czy lepiej jej nie umieszczać?

sed
źródło
7
Jeśli sprawdzasz tylko błędy w pierwszym „jeśli”, lepiej nie dodawaj słowa „else”, ponieważ błędów nie należy uważać za część rzeczywistej logiki.
Mert Akcakaya,
2
osobiście bardziej martwi mnie funkcja powodująca skutki uboczne, ale myślę, że to tylko źle wybrany przykład?
jk.
14
Dla mnie pierwsza wersja jest jak odwoływanie wszystkich studentów w sali, którzy nie kończą swoją pracę domową, a następnie raz ich nie ma, mówiąc do reszty studentów „Teraz, jeśli nie zakończy swoją pracę domową ...”. Ma to sens, ale jest niepotrzebne. Ponieważ warunek nie jest już tak naprawdę warunkowy, zwykle go odrzucam else.
Nicole,
1
Czym jest „Zrób coś więcej”, co chcesz zrobić? To może całkowicie zmienić sposób projektowania funkcji.
Benedykt

Odpowiedzi:

81

Przykład 2 jest znany jako blok ochronny. Lepiej jest odpowiednio wcześnie zwrócić / wyrzucić wyjątek, jeśli coś poszło nie tak (zły parametr lub nieprawidłowy stan). W normalnym przepływie logicznym lepiej jest użyć przykładu 1

Kemoda
źródło
+1 - dobre rozróżnienie i na co zamierzam odpowiedzieć.
Telastyn
3
@ pR0Ps ma tę samą odpowiedź poniżej i podaje przykład kodu wyjaśniający, dlaczego ostatecznie jest to czystsza ścieżka do naśladowania. +1 za dobrą odpowiedź.
To pytanie zostało zadane wiele razy na SO i choć może być sporne, jest to dobra odpowiedź. Wiele osób przeszkolonych do powrotu dopiero od końca ma pewne problemy z tą koncepcją.
Bill K
2
+1. Niewłaściwe unikanie bloków ochronnych powoduje kod strzałki.
Brian,
Czy oba przykłady nie pokazują blokady ochronnej?
ernie,
44

Moim osobistym stylem jest używanie pojedynczego ifdla bloków ochronnych i if/ elsew rzeczywistym kodzie przetwarzania metody.

W tym przypadku używasz myString == nullwarunku ochronnego, więc chciałbym użyć jednego ifwzorca.

Rozważ kod, który jest nieco bardziej skomplikowany:

Przykład 1:

public bool MyFunction(myString: string){

    //guard block
    if (myString == null){
        return false;
    }
    else{
        //processing block
        myString = escapedString(myString);

        if (myString == "foo"){
            //some processing here
            return false;
        }
        else{
            myString = "Name " + myString;
            //other stuff
            return true;
        }
    }
}

Przykład 2:

public bool MyFunction(myString: string){

    //guard block
    if (myString == null){
        return false;
    }

    //processing block
    myString = escapedString(myString);

    if (myString == "foo"){
        //some processing here
        return false;
    }
    else{
        myString = "Name " + myString;
        //other stuff
        return true;
    }
}

W przykładzie 1 zarówno strażnik, jak i reszta metody są w formie if/ else. Porównaj to z przykładem 2, w którym blok ochronny jest w pojedynczej ifformie, podczas gdy reszta metody używa formy if/ else. Osobiście uważam przykład 2 za łatwiejszy do zrozumienia, podczas gdy przykład 1 wygląda na niechlujny i nadmiernie wcięty.

Zauważ, że jest to wymyślony przykład i że możesz użyć else ifinstrukcji, aby go wyczyścić, ale chcę pokazać różnicę między blokami ochronnymi a rzeczywistym kodem przetwarzania funkcji.

Przyzwoity kompilator i tak powinien wygenerować te same dane wyjściowe dla obu. Jedynym powodem użycia jednego lub drugiego jest osobiste preferencje lub dostosowanie się do stylu istniejącego kodu.

pR0Ps
źródło
18
Przykład 2 byłby jeszcze łatwiejszy do odczytania, jeśli pozbyłeś się drugiego.
briddums,
18

osobiście wolę drugą metodę. Wydaje mi się, że jest krótszy, ma mniej wcięć i jest łatwiejszy do odczytania.

GSto
źródło
+1 za mniej wcięć. Ten gest jest nieprzyjemny, jeśli masz wiele czeków
d.raev
15

Moja osobista praktyka jest następująca:

  • Nie lubię funkcji z kilkoma punktami wyjścia, trudno mi było utrzymać i śledzić, modyfikacje kodu czasami psują wewnętrzną logikę, ponieważ z natury jest nieco niechlujna. Gdy jest to złożone obliczenie, tworzę wartość zwracaną na początku i zwracam na końcu. Zmusza mnie to do uważnego śledzenia każdej ścieżki if-else, przełącznika itp., Ustawiania wartości poprawnie w odpowiednich lokalizacjach. Poświęcam również trochę czasu na podjęcie decyzji, czy ustawić domyślną wartość zwracaną, czy pozostawić ją niezainicjowaną na początku. Ta metoda pomaga również, gdy zmienia się logika lub typ lub znaczenie zwracanej wartości.

Na przykład:

public bool myFunction()
{
   // First parameter loading
   String myString = getString();

   // Location of "quick exits", see the second example
   // ...

   // declaration of external resources that MUST be released whatever happens
   // ...

   // the return variable (should think about giving it a default value or not) 
   // if you have no default value, declare it final! You will get compiler 
   // error when you try to set it multiple times or leave uninitialized!
   bool didSomething = false;

   try {
     if (myString != null)
     {
       myString = "Name " + myString;
       // Do something more here...

       didSomething = true;
     } else {
       // get other parameters and data
       if ( other conditions apply ) {
         // do something else
         didSomething = true;
       }
     }

     // Edit: previously forgot the most important advantage of this version
     // *** HOUSEKEEPING!!! ***

   } finally {

     // this is the common place to release all resources, reset all state variables

     // Yes, if you use try-finally, you will get here from any internal returns too.
     // As I said, it is only my taste that I like to have one, straightforward path 
     // leading here, and this works even if you don't use the try-finally version.

   }

   return didSomething;
}
  • Jedyny wyjątek: „szybkie wyjście” na początku (lub w rzadkich przypadkach w trakcie procesu). Jeśli rzeczywista logika obliczeniowa nie jest w stanie poradzić sobie z pewną kombinacją parametrów wejściowych i stanów wewnętrznych lub ma łatwe rozwiązanie bez uruchamiania algorytmu, nie pomaga zamknięcie całego kodu (czasem głębokiego) w blokach. Jest to „wyjątkowy stan”, nie będący częścią podstawowej logiki, więc muszę wyjść z obliczeń, jak tylko go wykryję. W tym przypadku nie ma innej gałęzi, w normalnych warunkach wykonanie po prostu trwa. (Oczywiście „wyjątkowy stan” jest lepiej wyrażony poprzez zgłoszenie wyjątku, ale czasami jest to przesada.)

Na przykład:

public bool myFunction()
{
   String myString = getString();

   if (null == myString)
   {
     // there is nothing to do if myString is null
     return false;
   } 

   myString = "Name " + myString;
   // Do something more here...

   // not using return value variable now, because the operation is straightforward.
   // if the operation is complex, use the variable as the previous example.

   return true;
}

Reguła „jednego wyjścia” pomaga również wtedy, gdy obliczenia wymagają zewnętrznych zasobów, które musisz zwolnić, lub stwierdza, że ​​musisz zresetować przed opuszczeniem funkcji; czasami są one dodawane później podczas programowania. Przy wielu wyjściach w algorytmie znacznie trudniej jest poprawnie rozszerzyć wszystkie gałęzie. (A jeśli mogą wystąpić wyjątki, wydanie / reset należy również umieścić w bloku końcowym, aby uniknąć skutków ubocznych w rzadkich wyjątkowych przypadkach ...).

Twoja sprawa wydaje się należeć do kategorii „szybkie wyjście przed prawdziwą pracą” i napisałbym ją tak, jak w twojej wersji z Przykładu 2.

Lorand Kedves
źródło
9
+1 za „Nie lubię funkcji z kilkoma punktami wyjścia”
Corv1nus
@LorandKedves - dodał przykład - mam nadzieję, że nie masz nic przeciwko.
Matthew Flynn
@MatthewFlynn dobrze, jeśli nie masz nic przeciwko temu, że jest odwrotnie niż sugerowałem na końcu mojej odpowiedzi ;-) Kontynuuję przykład, mam nadzieję, że w końcu będzie dobrze dla nas obu :-)
Lorand Kedves
@MatthewFlynn (przepraszam, jestem po prostu drańcem z kraba i dziękuję za pomoc w wyjaśnieniu mojej opinii. Mam nadzieję, że podoba ci się obecna wersja.)
Lorand Kedves
13
Z funkcji z wieloma punktami wyjścia i funkcjami ze zmienną zmienną wynikową, ta pierwsza wydaje mi się zdecydowanie mniejszym z dwóch złych.
Jon Purdy,
9

Wolę stosować bloki ochronne tam, gdzie mogę, z dwóch powodów:

  1. Pozwalają na szybkie wyjście z powodu określonych warunków.
  2. Usuń konieczność skomplikowanych i niepotrzebnych instrukcji w dalszej części kodu.

Ogólnie rzecz biorąc wolę widzieć metody, w których podstawowa funkcjonalność metody jest jasna i minimalna. Bloki ochronne pomagają wizualnie to zrobić.

drekka
źródło
5

Podoba mi się podejście „Fall Through”:

public bool MyFunction()
{
   string myString = GetString();

   if (myString != null)
   {
     myString = "Name " + myString;
     return true;
    }
    return false;
}

Akcja ma określony warunek, wszystko inne to tylko domyślny powrót „przechodzący”.

Ciemna noc
źródło
1

Jeśli mam jeden warunek if, nie spędziłbym zbyt wiele czasu na rozmyślaniu o tym stylu. Ale jeśli mam wiele warunków ochrony, wolałbym style2

Zrób to. Załóżmy, że testy są złożone i naprawdę nie chcesz ich wiązać w jeden warunek jeśli-ORed, aby uniknąć złożoności:

//Style1
if (this1 != Right)
{ 
    return;
}
else if(this2 != right2)
{
    return;
}
else if(this3 != right2)
{
    return;
}
else
{
    //everything is right
    //do something
    return;
}

przeciw

//Style 2
if (this1 != Right)
{ 
   return;
}
if(this2 != right2)
{
    return;
}
if(this3 != right2)
{
    return;
}


//everything is right
//do something
return;

Oto dwie główne zalety

  1. Oddzielasz kod w jednej funkcji na dwa wizualnie logiczne bloki: górny blok walidacji (warunki ochrony) i dolny blok uruchamialnego kodu.

  2. Jeśli musisz dodać / usunąć jeden warunek, zmniejszasz swoje szanse na zepsucie całej drabiny if-elseif-else.

Kolejną niewielką zaletą jest to, że masz o jeden zestaw mniejszej liczby aparatów ortodontycznych.

DPD
źródło
0

Powinno to być kwestią, która brzmi lepiej.

If condition
  do something
else
  do somethingelse

wyraża się wtedy lepiej

if condition
  do something
do somethingelse

w przypadku małych metod nie będzie to wielka różnica, ale w przypadku większych metod złożonych może być trudniejszy do zrozumienia, ponieważ nie będzie odpowiednio oddzielny

José Valente
źródło
3
Jeśli metoda jest tak długa, że ​​druga forma jest trudna do zrozumienia, jest zbyt długa.
kevin cline
7
Twoje dwa przypadki są równoważne tylko wtedy, gdy do somethingzawierają zwrot. Jeśli nie, drugi kod zostanie wykonany do somethingelseniezależnie od ifpredykatu, który nie działa w taki sposób, jak pierwszy blok.
Adnan,
Są również tym, co OP wyjaśniał w swoich przykładach. Wystarczy podążać za pytaniem
José Valente,
0

Uważam przykład 1 za irytujący, ponieważ brakująca instrukcja return na końcu funkcji zwracającej wartość natychmiast wyzwala flagę „Czekaj, coś jest nie tak”. W takich przypadkach wybrałbym przykład 2.

Jednak zwykle jest więcej zaangażowanych, w zależności od celu funkcji, np. Obsługa błędów, rejestrowanie itp. Więc jestem z odpowiedzią Loranda Kedvesa na ten temat i zwykle mam jeden punkt wyjścia na końcu, nawet kosztem dodatkowej zmiennej flagi. W większości przypadków ułatwia konserwację i późniejsze rozszerzenia.

Bezpieczne
źródło
0

Kiedy twoja ifinstrukcja zawsze zwraca, nie ma powodu, aby używać innego dla pozostałego kodu w funkcji. Takie postępowanie dodaje dodatkowe linie i dodatkowe wcięcia. Dodanie niepotrzebnego kodu utrudnia czytanie i, jak wiadomo, czytanie kodu jest trudne .

briddums
źródło
0

W twoim przykładzie elsejest to oczywiście niepotrzebne, ALE ...

Podczas szybkiego przeglądania linii kodu oczy zwykle patrzą na nawiasy klamrowe i wgłębienia, aby uzyskać pojęcie o przepływie kodu; zanim faktycznie osiądą na samym kodzie. Dlatego pisanie elsei wstawianie nawiasów klamrowych i wcięć wokół drugiego bloku kodu sprawia, że ​​czytelnik szybciej widzi, że jest to sytuacja „A lub B”, w której jeden lub drugi blok będzie działał. Oznacza to, że czytelnik widzi nawiasy klamrowe i wgłębienia, ZANIM zobaczą returnpo inicjałach if.

Moim zdaniem jest to jeden z tych rzadkich przypadków, w których dodanie czegoś nadmiarowego do kodu sprawia, że ​​jest on WIĘCEJ czytelny, a nie mniej.

Dawood ibn Kareem
źródło
0

Wolałbym przykład 2, ponieważ natychmiast widać, że funkcja coś zwraca . Ale nawet więcej, wolałbym wrócić z jednego miejsca, takiego jak to:

public bool MyFunction()
{
    bool result = false;

    string myString = GetString();

    if (myString != nil) {
        myString = "Name " + myString;

        result = true;
    }

    return result;
}

W tym stylu mogę:

  1. Zobacz od razu, że coś zwracam.

  2. Złap wynik każdego wywołania funkcji za pomocą tylko jednego punktu przerwania.

Caleb
źródło
Jest to podobne do tego, jak podejdę do problemu. Jedyną różnicą jest to, że użyłbym zmiennej wynikowej, aby przechwycić ocenę myString i użyć jej jako wartości zwracanej.
Chuck Conway,
@ChuckConway zgodził się, ale starałem się trzymać prototypu OP.
Caleb
0

Mój osobisty styl zwykle odchodzi

function doQuery(string) {
    if (!query(string)) {
        query("ABORT");
        return false;
    } // else
    if(!anotherquery(string) {
        query("ABORT");
        return false;
    } // else
    return true;
}

Używanie skomentowanych elseinstrukcji w celu wskazania przebiegu programu i zachowania jego czytelności, ale unikanie masywnych wcięć, które mogą łatwo dotrzeć na ekran, jeśli zaangażowanych jest wiele kroków.

Shadur
źródło
1
Osobiście mam nadzieję, że nigdy nie będę musiał pracować z twoim kodem.
CVn
Jeśli masz wiele kontroli, ta metoda może trochę potrwać. Zwracanie z każdej klauzuli if jest podobne do używania instrukcji goto w celu pominięcia części kodu.
Chuck Conway,
Gdyby to był wybór między tym a kodem z elsedołączonymi klauzulami, wybrałbym to drugie, ponieważ kompilator skutecznie je również zignoruje. Choć zależałoby to od tego, by else if nie zwiększać wcięcia więcej niż oryginał - jeśli tak , zgodziłbym się z tobą. Ale moim wyborem byłoby elsecałkowite porzucenie, gdyby ten styl był dozwolony.
Mark Hurd,
0

Napisałbym

public bool SetUserId(int id)
{
   // Get user name from id
   string userName = GetNameById(id);

   if (userName != null)
   {
       // update local ID and userName
       _id = id;
       _userNameLabelText = "Name: " + userName;
   }

   return userName != null;
}

Wolę ten styl, ponieważ najbardziej oczywiste jest, że wrócę userName != null.

tia
źródło
1
Pytanie brzmi: dlaczego wolisz ten styl?
Caleb,
... Szczerze mówiąc, nie jestem pewien, dlaczego w tym przypadku chcesz łańcucha, nie wspominając o tym, dlaczego dodajesz dodatkowy test boolowski na końcu, którego tak naprawdę nie potrzebujesz z żadnego powodu.
Shadur
@Shadur Jestem świadomy dodatkowego testu boolowskiego i niewykorzystanego myStringwyniku. Właśnie kopiuję funkcję z OP. Zamienię to na coś, co wygląda bardziej realistycznie. Test boolowski jest trywialny i nie powinien być problemem.
tia,
1
@Shadur Nie zajmujemy się optymalizacją, prawda? Chodzi mi o to, aby mój kod był łatwy do odczytania i obsługi, a osobiście zapłaciłbym to kosztem jednej referencyjnej kontroli zerowej.
tia
1
@ tia Lubię ducha twojego kodu. Zamiast podwójnej oceny userName przechwycę pierwszą ocenę w zmiennej i zwrócę ją.
Chuck Conway,
-1

Moją ogólną zasadą jest albo wykonanie zwrotu „bez” (głównie w przypadku błędów), albo wykonanie pełnego łańcucha „jeśli-inaczej-jeśli” przy wszystkich możliwościach.

W ten sposób unikam zbytniej fantazji przy rozgałęzianiu, ponieważ za każdym razem, gdy to robię, koduję logikę aplikacji w moich ifs, co utrudnia ich utrzymanie (na przykład, jeśli wyliczenie OK lub ERR i piszę wszystkie moje ifs korzystając z faktu, że! OK <-> ERR staje się problemem w tyłku, aby dodać trzecią opcję do wyliczenia następnym razem).

W twoim przypadku użyłbym na przykład zwykłego, jeśli „return if null” z pewnością będzie wspólnym wzorcem i jest mało prawdopodobne, że będziesz musiał się martwić o trzecią możliwość (oprócz null / not null) w przyszłości.

Jeśli jednak test był czymś, co sprawdziło bardziej szczegółową kontrolę danych, popełniłbym błąd w kierunku pełnego „jeśli-inaczej”.

hugomg
źródło
-1

Myślę, że w Przykładzie 2 jest wiele pułapek, które mogą prowadzić do niezamierzonego kodu. Pierwszy punkt zainteresowania jest tutaj oparty na logice otaczającej zmienną „myString”. Dlatego, mówiąc wprost, wszystkie testy warunków powinny odbywać się w bloku kodu uwzględniającym znaną i domyślną / nieznaną logikę.

Co jeśli później kod został przypadkowo wprowadzony do przykładu 2, który znacząco zmienił wynik:

   if (myString == null)
   {
      return false;
   }

   //add some v1 update code here...
   myString = "And the winner is: ";
   //add some v2 update code here...
   //buried in v2 updates the following line was added
   myString = null;
   //add some v3 update code here...
   //Well technically this should not be hit because myString = null
   //but we already passed that logic
   myString = "Name " + myString;
   // Do something more here...
   return true;

Myślę, że elseblok bezpośrednio po sprawdzeniu wartości zerowej spowodowałby, że programiści, którzy dodali ulepszenia do przyszłych wersji, dodają całą logikę razem, ponieważ teraz mamy ciąg logiki, który był niezamierzony dla oryginalnej reguły (zwracany, jeśli wartość jest zero).

Wierzę w to mocno niektórym z wytycznych C # na Codeplex (link do tego tutaj: http://csharpguidelines.codeplex.com/ ), które zawierają następujące informacje:

„Dodaj opisowy komentarz, jeśli domyślny blok (inny) ma być pusty. Ponadto, jeśli ten blok nie zostanie osiągnięty, wyrzuć wyjątek InvalidOperationException, aby wykryć przyszłe zmiany, które mogą wystąpić w istniejących przypadkach. Zapewnia to lepszy kod, ponieważ wszystkie ścieżki, które kod może przemierzać, zostały przemyślane. ”

Myślę, że dobrą praktyką programistyczną jest stosowanie takich bloków logicznych, aby zawsze dodawać blok domyślny (jeśli-inaczej, case: default), aby jawnie uwzględniać wszystkie ścieżki kodu i nie pozostawiać kodu otwartego na niezamierzone konsekwencje logiczne.

atconway
źródło
W jaki sposób brak innej w przykładzie 2 nie uwzględnia wszystkich przypadków? Zamierzonym rezultatem JEST kontynuacja wykonywania. Dodanie klauzuli else w tym przypadku tylko zwiększa złożoność metody.
Chuck Conway,
Nie zgadzam się z tym, że nie wszystkie zainteresowane logiki są w zwięzłym i deterministycznym bloku. Nacisk kładziony jest na manipulowanie myStringwartością, a kod następujący po ifbloku jest logiką wynikową, jeśli łańcuch! = Null. Dlatego, ponieważ nacisk kładziony jest na dodatkową logikę manipulującą tą konkretną wartością, myślę, że powinna ona być zamknięta w elsebloku. W przeciwnym razie otwiera to potencjał, ponieważ pokazałem, że mimowolnie rozdzielam logikę i wprowadzam niezamierzone konsekwencje. Może się to nie zdarzać cały czas, ale tak naprawdę było to pytanie oparte na opinii najlepszych praktyk .
atconway