Ocena zwarcia, czy to zła praktyka?

Coś, o czym wiem od jakiegoś czasu, ale nigdy nie brałem pod uwagę, że w większości języków można nadać priorytet operatorom w instrukcji if na podstawie ich kolejności. Często używam tego jako sposobu zapobiegania wyjątkom o zerowym odwołaniu, np .:

if (smartphone != null && smartphone.GetSignal() > 50)
{
   // Do stuff
}

W takim przypadku kod spowoduje najpierw sprawdzenie, czy obiekt nie jest pusty, a następnie użycie tego obiektu, wiedząc, że istnieje. Język jest sprytny, ponieważ wie, że jeśli pierwsza instrukcja jest fałszywa, nie ma sensu nawet oceny drugiej instrukcji, dlatego wyjątek odniesienia zerowego nigdy nie jest generowany. To działa tak samo dla operatorów andi or.

Może to być przydatne również w innych sytuacjach, takich jak sprawdzanie, czy indeks znajduje się w granicach tablicy i taka technika może być wykonywana w różnych językach, o ile mi wiadomo: Java, C #, C ++, Python i Matlab.

Moje pytanie brzmi: czy ten rodzaj kodu reprezentuje złą praktykę? Czy ta zła praktyka wynika z jakiegoś ukrytego problemu technicznego (tj. Może to ostatecznie spowodować błąd), czy może prowadzi do problemu czytelności dla innych programistów? Czy to może być mylące?

Nie, to nie jest zła praktyka. Poleganie na zwarciu warunków warunkowych jest powszechnie akceptowaną, przydatną techniką - o ile używasz języka, który gwarantuje takie zachowanie (który obejmuje zdecydowaną większość współczesnych języków).

Twój przykład kodu jest dość jasny i rzeczywiście jest to często najlepszy sposób na napisanie go. Alternatywy (takie jak ifinstrukcje zagnieżdżone ) byłyby bardziej skomplikowane, a przez to trudniejsze do zrozumienia.

Kilka zastrzeżeń:

Stosuj zdrowy rozsądek w odniesieniu do złożoności i czytelności

Następujące nie jest tak dobrym pomysłem:

if ((text_string != null && replace(text_string,"$")) || (text_string = get_new_string()) != null)

Podobnie jak wiele „sprytnych” sposobów zmniejszania liczby wierszy w kodzie, nadmierne poleganie na tej technice może skutkować trudnym do zrozumienia kodem podatnym na błędy.

Jeśli potrzebujesz obsługiwać błędy, często istnieje lepszy sposób

Twój przykład jest w porządku, o ile jest to normalny stan programu, w którym smartfon ma wartość zerową. Jeśli jednak jest to błąd, należy zastosować inteligentniejszą obsługę błędów.

Unikaj powtarzanych kontroli tego samego stanu

Jak podkreśla Neil, wiele powtarzających się kontroli tej samej zmiennej w różnych warunkach warunkowych jest najprawdopodobniej dowodem wad projektowych. Jeśli masz dziesięć różnych instrukcji pokropionych przez twój kod, które nie powinny być uruchamiane, jeśli tak smartphonejest null, zastanów się, czy istnieje lepszy sposób na poradzenie sobie z tym niż sprawdzanie zmiennej za każdym razem.

Tak naprawdę nie chodzi o zwarcie; jest to bardziej ogólny problem powtarzalnego kodu. Warto jednak wspomnieć, ponieważ kod często zawiera wiele powtarzających się instrukcji, takich jak przykładowa instrukcja.

Powiedzmy, że używasz języka w stylu C bez &&i musisz wykonać równoważny kod jak w pytaniu.

Twój kod to:

if(smartphone != null)
{
  if(smartphone.GetSignal() > 50)
  {
    // Do stuff
  }
}

Ten wzór bardzo by się pojawił.

Wyobraźmy sobie teraz wersję 2.0 naszego hipotetycznego języka &&. Pomyśl, jakie to fajne!

&&jest uznanym, idiomatycznym sposobem robienia tego, co robi mój powyższy przykład. Nie jest to zła praktyka, aby go używać, w rzeczywistości jest to zła praktyka, aby nie używać go w takich przypadkach: Doświadczony programista w takim języku zastanawiałby się, gdzie elsebył lub inny powód, aby nie robić rzeczy w normalny sposób.

Język jest sprytny, ponieważ wie, że jeśli pierwsza instrukcja jest fałszywa, nie ma sensu nawet oceny drugiej instrukcji, dlatego wyjątek odniesienia zerowego nigdy nie jest generowany.

Nie, jesteś sprytny, ponieważ wiedziałeś, że jeśli pierwsze zdanie jest fałszywe, nie ma sensu nawet oceniać drugiego zdania. Język jest głupi jak pudełko kamieni i zrobiłeś to, co mu kazałeś. (John McCarthy był jeszcze bardziej sprytny i zdał sobie sprawę, że ocena zwarcia byłaby przydatna dla języków).

Różnica między byciem sprytnym a językiem sprytnym jest ważna, ponieważ dobre i złe praktyki często sprowadzają się do bycia tak sprytnym, jak to konieczne, ale nie bardziej sprytnym.

Rozważać:

if(smartphone != null && smartphone.GetSignal() > ((range = (range != null ? range : GetRange()) != null && range.Valid ? range.MinSignal : 50)

To rozszerza twój kod, aby sprawdzić range. Jeśli rangewartość jest zerowa, wówczas próbuje ustawić ją przez wywołanie, GetRange()chociaż może to się nie powieść, więc rangemoże nadal być zerowe. Jeśli po tym czasie zakres nie jest zerowy i jest Validto jego MinSignalwłaściwość jest używana, w przeciwnym razie używana jest wartość domyślna 50.

Zależy to &&również od tego , ale umieszczenie go w jednym wierszu jest prawdopodobnie zbyt sprytne (nie jestem w 100% pewien, że mam rację i nie zamierzam dwukrotnie sprawdzać, ponieważ ten fakt pokazuje moją rację).

Nie jest &&to jednak problemem, ale umiejętność użycia go w celu umieszczenia dużej ilości jednego wyrażenia (dobra rzecz) zwiększa naszą zdolność do niepotrzebnego pisania trudnych do zrozumienia wyrażeń (zła rzecz).

Również:

if(smartphone != null && (smartphone.GetSignal() == 2 || smartphone.GetSignal() == 5 || smartphone.GetSignal() == 8 || smartPhone.GetSignal() == 34))
{
  // do something
}

Tutaj łączę użycie &&z kontrolą pewnych wartości. Nie jest to realistyczne w przypadku sygnałów telefonicznych, ale pojawia się w innych przypadkach. Oto przykład tego, że nie jestem wystarczająco sprytny. Gdybym zrobił następujące rzeczy:

if(smartphone != null)
{
  switch (smartphone.GetSignal())
  {
    case 2: case 5: case 8: case 34:
      // Do something
      break;
  }
}

Zyskałbym zarówno na czytelności, jak i prawdopodobnie na wydajności (wiele wywołań do GetSignal()prawdopodobnie nie zostałoby zoptymalizowanych).

Tutaj znowu problemem nie jest &&wzięcie tego konkretnego młotka i postrzeganie wszystkiego innego jako gwoździa; nieużywanie go pozwoli mi zrobić coś lepszego niż to.

Ostatnim przypadkiem, który odchodzi od najlepszych praktyk, jest:

if(a && b)
{
  //do something
}

W porównaniu do:

if(a & b)
{
  //do something
}

Klasycznym argumentem przemawiającym za tym, dlaczego możemy faworyzować to drugie, jest pewien efekt uboczny przy ocenie b, czy chcemy, czy ajest to prawda, czy nie. Nie zgadzam się z tym: jeśli ten efekt uboczny jest tak ważny, to należy go wprowadzić w osobnym wierszu kodu.

Jednak pod względem wydajności jedno z tych dwóch prawdopodobnie będzie lepsze. Pierwszy oczywiście wykona mniej kodu (nie oceniając bwcale w jednej ścieżce kodu), co może zaoszczędzić nam czasu potrzebnego do oceny b.

Pierwszy ma jednak jeszcze jedną gałąź. Zastanów się, czy przepiszemy go w naszym hipotetycznym języku w stylu C bez &&:

if(a)
{
  if(b)
  {
    // Do something
  }
}

Ten dodatek ifjest ukryty w naszym użyciu &&, ale nadal tam jest. I jako taki jest to przypadek, w którym dochodzi do przewidywania gałęzi, a zatem potencjalnie może to wynikać z błędnej prognozy gałęzi.

Z tego powodu if(a & b)w niektórych przypadkach kod może być bardziej wydajny.

Tutaj powiem, że if(a && b)nadal jest to najlepsza praktyka na początek: bardziej powszechna, jedyna możliwa do wykonania w niektórych przypadkach (gdzie bbędzie błąd, jeśli ajest fałszywa) i szybsza częściej niż nie. Warto zauważyć, if(a & b)że w niektórych przypadkach jest to często przydatna optymalizacja.

Możesz pójść o krok dalej, aw niektórych językach jest to idiomatyczny sposób robienia rzeczy: możesz używać ewaluacji zwarć poza instrukcjami warunkowymi, a one same stają się formą instrukcji warunkowych. Np. W Perlu idiomatyczne jest, aby funkcje zwracały coś fałszywego w przypadku niepowodzenia (i coś prawdziwego w przypadku sukcesu) i coś w rodzaju

open($handle, "<", "filename.txt") or die "Couldn't open file!";

jest idiomatyczny. openzwraca coś niezerowego w przypadku sukcesu (tak, że dieczęść po nim ornigdy się nie zdarza), ale niezdefiniowanego w przypadku niepowodzenia, a następnie wezwanie do wykonania diema miejsce (jest to sposób Perla na zgłoszenie wyjątku).

Nie ma problemu w językach, w których wszyscy to robią.

Chociaż ogólnie zgadzam się z odpowiedzią dan1111 , jest jeden szczególnie ważny przypadek, którego nie obejmuje: przypadek, w którym smartphonejest używany jednocześnie. W tym scenariuszu tego rodzaju wzorzec jest dobrze znanym źródłem trudnych do znalezienia błędów.

Problem polega na tym, że ocena zwarcia nie jest atomowa. Twój wątek może sprawdzić, czy smartphoneznaczy null, inny wątek może przyjść i unieważnić ją, a następnie swoją nić natychmiast próbuje GetSignal()i wysadza.

Ale oczywiście, tylko robi, że kiedy gwiazdy wyrównać i robieniu podaje jest po prostu w porządku.

Więc chociaż ten rodzaj kodu jest bardzo powszechny i ​​użyteczny, ważne jest, aby wiedzieć o tym zastrzeżeniu, abyś mógł dokładnie zapobiec temu nieprzyjemnemu błędowi.

Istnieje wiele sytuacji, w których chcę najpierw sprawdzić jeden warunek, a drugi chcę sprawdzić tylko wtedy, gdy pierwszy warunek się powiedzie. Czasami wyłącznie ze względu na wydajność (ponieważ nie ma sensu sprawdzanie drugiego warunku, jeśli pierwszy już się nie powiedzie), czasami dlatego, że w przeciwnym razie mój program się zawiesi (twoje sprawdzenie NULL jako pierwsze), czasami ponieważ w przeciwnym razie wyniki byłyby okropne. (JEŻELI (chcę wydrukować dokument) ORAZ (drukowanie dokumentu nie powiodło się)) NASTĘPNIE wyświetla komunikat o błędzie - nie chcesz wydrukować dokumentu i sprawdź, czy drukowanie nie powiodło się, jeśli nie chcesz wydrukować dokumentu w pierwsze miejsce!

Tak więc ogromna potrzeba gwarantowanej oceny zwarć wyrażeń logicznych. Nie było go w Pascalu (który nie miał gwarantowanej oceny zwarć), co było dużym bólem; Pierwszy raz widziałem to w Adzie i C.

Jeśli naprawdę uważasz, że jest to „zła praktyka”, weź trochę umiarkowanie złożonego kodu, przepisz go, aby działał dobrze bez oceny zwarć, i wróć i powiedz nam, jak ci się podobało. To tak, jakby powiedzieć, że oddychanie wytwarza dwutlenek węgla i pytanie, czy oddychanie jest złą praktyką. Spróbuj bez niego przez kilka minut.

Jedna uwaga, nie wymieniona w innych odpowiedziach: czasami posiadanie tych kontroli może wskazywać na możliwe refaktoryzacja do wzorca projektowego Null Object . Na przykład:

if (currentUser && currentUser.isAdministrator()) 
  doSomething();

Można uprościć:

if (currentUser.isAdministrator())
  doSomething ();

Jeśli currentUserdomyślnie jest ustawiony jako „anonimowy użytkownik” lub „zerowy użytkownik” z rezerwową implementacją, jeśli użytkownik nie jest zalogowany.

Nie zawsze zapach kodu, ale coś do rozważenia.

Będę niepopularny i powiem tak, to zła praktyka. JEŻELI funkcjonalność kodu polega na nim w celu wdrożenia logiki warunkowej.

to znaczy

 if(quickLogicA && slowLogicB) {doSomething();}

jest dobrze, ale

if(logicA && doSomething()) {andAlsoDoSomethingElse();}

jest zły i na pewno nikt się z nim nie zgodzi ?!

if(doSomething() || somethingElseIfItFailed() && anotherThingIfEitherWorked() & andAlwaysThisThing())
{/* do nothing */}

Uzasadnienie:

Błędy w kodzie, jeśli obie strony są oceniane, zamiast po prostu nie wykonywać logiki. Argumentowano, że nie jest to problem, operator „i” jest zdefiniowany w języku c #, więc znaczenie jest jasne. Uważam jednak, że to nieprawda.

Powód 1. Historyczny

Zasadniczo polegasz na (jak pierwotnie zamierzano) optymalizacji kompilatora, aby zapewnić funkcjonalność swojego kodu.

Chociaż w c # specyfikacja języka gwarantuje, że operator logiczny i operator zwarły. Nie dotyczy to wszystkich języków i kompilatorów.

wikipeda wymienia różne języki i ich zwarcia http://en.wikipedia.org/wiki/Short-circuit_evaluation, ponieważ istnieje wiele sposobów. I kilka dodatkowych problemów, o których tu nie mówię.

W szczególności FORTRAN, Pascal i Delphi mają flagi kompilatora, które przełączają to zachowanie.

Dlatego: może się okazać, że kod działa podczas kompilacji do wydania, ale nie do debugowania lub z alternatywnym kompilatorem itp.

Powód 2. Różnice językowe

Jak widać z wikipedii, nie wszystkie języki mają takie samo podejście do zwarć, nawet jeśli określają, w jaki sposób operatorzy logiczni powinni sobie z tym poradzić.

W szczególności operator „i” VB.Net nie powoduje zwarcia, a TSQL ma pewne cechy szczególne.

Biorąc pod uwagę, że współczesne „rozwiązanie” może zawierać wiele języków, takich jak javascript, regex, xpath itp., Powinieneś wziąć to pod uwagę, wyjaśniając funkcjonalność kodu.

Powód 3. Różnice w języku

Na przykład wstawka VB jeśli zachowuje się inaczej niż jej. Ponownie we współczesnych aplikacjach Twój kod może się przemieszczać, na przykład kod z tyłu i wbudowany formularz internetowy, musisz zdawać sobie sprawę z różnic w znaczeniu.

Powód 4. Twój konkretny przykład zerowania sprawdzania parametru funkcji

To bardzo dobry przykład. Jest to coś, co wszyscy robią, ale w rzeczywistości jest to zła praktyka, gdy się nad tym zastanowić.

Tego rodzaju sprawdzanie jest bardzo częste, ponieważ znacznie zmniejsza liczbę linii kodu. Wątpię, by ktokolwiek przywołał to w recenzji kodu. (i oczywiście z komentarzy i ocen widać, że jest popularny!)

Jednak zawodzi najlepsza praktyka z więcej niż jednego powodu!

1. Jego bardzo jasne z wielu źródeł, że najlepszym rozwiązaniem jest, aby sprawdzić parametry dla ważności przed ich użyciem i rzucić wyjątek, jeśli są one nieważne. Fragment kodu nie pokazuje otaczającego kodu, ale prawdopodobnie powinieneś robić coś takiego:

   if (smartphone == null)
   {
       //throw an exception
   }
   // perform functionality

2. Dzwonisz funkcję z wewnątrz rachunku warunkowego. Chociaż nazwa Twojej funkcji sugeruje, że po prostu oblicza wartość, może ukrywać skutki uboczne. na przykład

   Smartphone.GetSignal() { this.signal++; return this.signal; }
   
   else if (smartphone.GetSignal() < 1)
   {
       // Do stuff 
   }
   else if (smartphone.GetSignal() < 2)
   {
       // Do stuff 
   }

   najlepsza praktyka sugeruje:

   var s = smartphone.GetSignal();
   if(s < 50)
   {
       //do something
   }

Jeśli zastosujesz te najlepsze praktyki do swojego kodu, zobaczysz, że Twoja szansa na uzyskanie krótszego kodu poprzez użycie ukrytego warunkowego znika. Najlepszą praktyką jest zrobienie czegoś , załatwienie sprawy, w której smartfon jest zerowy.

Myślę, że przekonasz się, że dotyczy to również innych powszechnych zastosowań. Musisz pamiętać, że mamy również:

warunkowe inline

var s = smartphone!=null ? smartphone.GetSignal() : 0;

i zerowa koalescencja

var s = smartphoneConnectionStatus ?? "No Connection";

które zapewniają podobną funkcjonalność krótkiego kodu z większą przejrzystością

liczne linki do poparcia wszystkich moich punktów:

https://stackoverflow.com/questions/1158614/what-is-the-best-practice-in-case-one-argument-is-null

Sprawdzanie poprawności parametrów konstruktora w C # - Najlepsze praktyki

Czy należy sprawdzić wartość zerową, jeśli nie spodziewa się wartości zerowej?

https://msdn.microsoft.com/en-us/library/seyhszts%28v=vs.110%29.aspx

https://stackoverflow.com/questions/1445867/why-would-a-language-not-use-short-circuit-evaluation

http://orcharddojo.net/orchard-resources/Library/DevelopmentGuidelines/BestPractices/CSharp

przykłady flag kompilatora wpływających na zwarcie:

Fortran: "sce | nosce Domyślnie kompilator dokonuje oceny zwarcia w wybranych wyrażeniach logicznych przy użyciu reguł XL Fortran. Określenie sce pozwala kompilatorowi używać reguł innych niż XL Fortran. Kompilator przeprowadzi ocenę zwarcia, jeśli pozwalają na to obecne reguły . Domyślnie jest nosce. ”

Pascal: „B - Flaga flagowa, która kontroluje ocenę zwarcia. Aby uzyskać więcej informacji na temat oceny zwarć, zobacz Kolejność oceny operandów operatorów diadycznych. Aby uzyskać więcej informacji, zobacz także opcję kompilatora -sc ( udokumentowane w Podręczniku użytkownika). ”

Delphi: „Delphi domyślnie obsługuje analizę zwarć. Można ją wyłączyć za pomocą dyrektywy kompilatora {$ BOOLEVAL OFF}.”