Jeśli jeszcze drabina ma spełnić wszystkie warunki - czy należy dodać zbędną klauzulę końcową?

10

To ostatnio dużo robię.

Przykład:

setCircle(circle, i, { current }) {
    if (i == current) {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
    } else if (i < current) {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
    } else if (i > current) {
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    }
}

Często drabina if / else jest znacznie bardziej skomplikowana niż ta ...

Zobacz ostatnią klauzulę? Jest zbędny. Drabina ma ostatecznie uchwycić wszystkie możliwe warunki. W ten sposób można go przepisać w ten sposób:

setCircle(circle, i, { current }) {
    if (i == current) {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
    } else if (i < current) {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
    } else {
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    }
}

Tak pisałem kod, ale nie lubię tego stylu. Moja skarga polega na tym, że warunek, w którym zostanie wykonana ostatnia część kodu, nie jest oczywisty z kodu. W ten sposób zacząłem pisać ten warunek, aby był bardziej widoczny.

Jednak:

  • Wyraźnie napisanie ostatecznego wyczerpującego warunku jest moim własnym pomysłem i mam złe doświadczenia z własnymi pomysłami - zwykle ludzie krzyczą na mnie o tym, jak okropne jest to, co robię - i (czasami dużo) później dowiaduję się, że to rzeczywiście było nieoptymalny;
  • Jedna wskazówka, dlaczego może to być zły pomysł: nie dotyczy JavaScript, ale w innych językach kompilatory mają tendencję do wydawania ostrzeżeń, a nawet błędów dotyczących kontrolowania osiągnięcia funkcji. Sugerowanie zrobienia czegoś takiego może nie być zbyt popularne lub robię to źle.
    • Skargi kompilatora sprawiły, że czasami napisałem końcowy warunek w komentarzu, ale myślę, że jest to okropne, ponieważ komentarze, w przeciwieństwie do kodu, nie mają wpływu na rzeczywistą semantykę programu:
    } else { // i > current
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    }

Czy coś brakuje? A może robienie tego, co opisałem, to zły pomysł?

gaazkam
źródło
+1 za wykonalną odpowiedź @ Christophe, że nadmiarowość w kodzie zwiększa szanse na defekty. Istnieje również znacznie mniejszy problem z wydajnością. Aby rozwinąć tę kwestię, w odniesieniu do omawianego przykładu kodu, moglibyśmy napisać serię if-else-if tak osobno, jeśli jest bez żadnych innych, ale generalnie nie zrobilibyśmy, ponieważ if-else daje czytelnikowi pojęcie wyłącznych alternatyw zamiast szeregu niezależnych warunków (co również byłoby mniej wydajne, ponieważ oznacza to, że wszystkie warunki należy ocenić nawet po jednym dopasowaniu).
Erik Eidt,
@ErikEidt>, ponieważ mówi to, że wszystkie warunki powinny być ocenione nawet po jednym dopasowaniu +1, chyba że wracasz z funkcji lub „przerywasz” pętlę (co nie ma miejsca w powyższym przykładzie).
Darek Nędza
1
+1, fajne pytanie. Nawiasem mówiąc, może Cię to zainteresować, że w obecności NaN przykład nie jest wyczerpujący.
monokomórka

Odpowiedzi:

6

Oba podejście jest ważne. Ale przyjrzyjmy się bliżej zaletom i wadom.

Dla ifłańcucha z tak trywialnymi warunkami, jak tutaj, tak naprawdę nie ma znaczenia:

  • w ostatecznym elserozrachunku dla czytelnika oczywiste jest, w jakim stanie zostaje wyzwolona reszta;
  • z finałem else if, dla czytelnika jest oczywiste, że nie elsepotrzebujesz żadnych dodatkowych , ponieważ wszystko to omówiłeś.

Istnieje jednak wiele ifłańcuchów, które opierają się na bardziej złożonych warunkach, łącząc stany kilku zmiennych, być może ze złożonym wyrażeniem logicznym. W tym przypadku jest to mniej oczywiste. A tutaj konsekwencja każdego stylu:

  • końcowy else: masz pewność, że jeden z oddziałów jest zajęty. Jeśli zdarzy się, że zapomniałeś o jednej sprawie, przejdzie ona przez ostatnią gałąź, więc podczas debugowania, jeśli wybrano ostatnią gałąź i oczekiwałeś czegoś innego, szybko się domyślisz.
  • końcowy else if: musisz wyprowadzić nadmiarowy warunek do kodu, a to stwarza potencjalne źródło błędów z ryzykiem nieobjęcia wszystkich przypadków. Ponadto, jeśli przegapiłeś przypadek, nic nie zostanie wykonane i może być trudniej dowiedzieć się, że czegoś brakowało (np. Jeśli niektóre zmienne, które miały być ustawione, zachowają wartości z poprzednich iteracji).

Zatem ostateczny stan nadmiarowy jest źródłem ryzyka. Właśnie dlatego wolałbym przejść do finału else.

Edycja: kodowanie o wysokiej niezawodności

Jeśli tworzysz z myślą o wysokiej niezawodności, możesz zainteresować się innym wariantem: uzupełnieniem zbędnego jawnego finału else iffinałem elsew celu wychwycenia nieoczekiwanych sytuacji.

To jest kodowanie obronne. Jest to zalecane przez niektóre specyfikacje bezpieczeństwa, takie jak SEI CERT lub MISRA . Niektóre narzędzia analizy statycznej implementują to jako regułę, która jest systematycznie sprawdzana (może to wyjaśniać ostrzeżenia kompilatora).

Christophe
źródło
7
Co jeśli po ostatecznym stanie redundancji dodam inny, który zawsze zgłasza wyjątek, który mówi: „Nie miałeś do mnie dotrzeć!” - Czy to rozwiąże niektóre problemy mojego podejścia?
gaazkam
3
@gaazkam tak, to bardzo defensywny styl kodowania. Nadal musisz obliczyć ostateczny warunek, ale w przypadku złożonych łańcuchów podatnych na błędy przynajmniej szybko się dowiesz. Jedyny problem, jaki widzę w tym wariancie, to to, że w przypadku oczywistego przypadku jest to trochę przesada.
Christophe,
@gaazkam Zredagowałem moją odpowiedź, aby odnieść się również do twojego dodatkowego pomysłu wymienionego w twoim komentarzu
Christophe
1
@gaazkam Zgłaszanie wyjątku jest dobre. Jeśli tak, nie zapomnij podać nieoczekiwanej wartości w wiadomości. Może być trudne do odtworzenia, a nieoczekiwana wartość może dostarczyć wskazówek na temat źródła problemu.
Martin Maat
1
Inną opcją jest umieszczenie assertfinału else if. Twój przewodnik po stylu może się jednak różnić w zależności od tego, czy jest to dobry pomysł. (Warunek „a” assertnigdy nie powinien być fałszywy, chyba że programista spieprzył. Więc to przynajmniej używa tej funkcji zgodnie z jej przeznaczeniem. Ale tak wiele osób niewłaściwie z niej korzysta, że ​​wiele sklepów zakazało jej wprost.)
Kevin
5

Coś, czego dotychczas brakuje w odpowiedziach, zależy od tego, jaki rodzaj awarii jest mniej szkodliwy.

Jeśli twoja logika jest dobra, tak naprawdę nie ma znaczenia, co robisz, ważne jest to, co się dzieje, jeśli masz błąd.

Pomijasz ostatni warunek: ostatnia opcja jest wykonywana, nawet jeśli nie jest to właściwe.

Po prostu dodajesz końcowy warunek: nie wykonuje żadnej opcji, w zależności od sytuacji może to po prostu oznaczać, że coś nie wyświetla się (niska szkoda), lub może oznaczać wyjątek zerowy odniesienia w pewnym późniejszym momencie (co może być debugowaniem ból.)

Dodajesz końcowy warunek i wyjątek: wyrzuca.

Musisz zdecydować, która z tych opcji jest najlepsza. W kodzie programistycznym uważam to za oczywiste - weźmy trzeci przypadek. Najprawdopodobniej jednak ustawiłbym koło.src na obraz błędu, a okrąg.alt na komunikat o błędzie przed wyrzuceniem - na wypadek, gdyby ktoś zdecydował się później wyłączyć twierdzenia, spowoduje to, że będzie on nieszkodliwy.

Kolejna rzecz do rozważenia - jakie są opcje odzyskiwania? Czasami nie masz ścieżki odzyskiwania. Moim zdaniem najlepszym tego przykładem jest pierwsza rakieta Ariane V. Wystąpił nieprzechwycony błąd / 0 (właściwie przepełnienie podziału), który spowodował zniszczenie wzmacniacza. W rzeczywistości kod, który się zawiesił, nie miał w tym momencie żadnego sensu, stał się dyskusyjny w chwili, gdy zapalono wzmacniacze przypinane taśmą. Gdy zaświecą się na orbitę lub boom, robisz, co możesz, nie można dopuścić do błędów. (Jeśli rakieta zbłądzi z tego powodu, zawodnik od bezpieczeństwa przekręca klucz.)

Loren Pechtel
źródło
4

To, co zalecam, to użycie assertoświadczenia w końcowej wersji, w jednym z tych dwóch stylów:

setCircle(circle, i, { current }) {
    if (i == current) {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
    } else if (i < current) {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
    } else {
        assert i > current
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    }
}

Lub stwierdzenie martwego kodu:

setCircle(circle, i, { current }) {
    if (i == current) {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
    } else if (i < current) {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
    } else if (i > current) {
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    } else {
        assert False, "Unreachable code"
    }
}

Narzędzie pokrycia kodu można często skonfigurować tak, aby ignorowało kod taki jak „aserse False” z raportu pokrycia.


Umieszczając warunek w asercji, skutecznie dokumentujesz warunek gałęzi jawnie, ale w przeciwieństwie do komentarza, warunek asercji można faktycznie sprawdzić i zakończy się niepowodzeniem, jeśli asercje będą włączone podczas programowania lub w trakcie produkcji (generalnie zalecam włączenie asercji) w produkcji, jeśli nie wpływają zbytnio na wydajność).

Lie Ryan
źródło
1
Nie podoba mi się twoja pierwsza opcja, druga jest o wiele wyraźniejsza, że ​​jest to przypadek niemożliwy.
Loren Pechtel
0

Zdefiniowałem makro „potwierdzone”, które ocenia warunek, aw kompilacji debugowania wpada do debugera.

Więc jeśli jestem w 100 procentach pewien, że jeden z trzech warunków musi być spełniony, piszę

If condition 1 ...
Else if condition 2 .,,
Else if asserted (condition3) ...

Dzięki temu jest wystarczająco jasne, że jeden warunek będzie spełniony i nie jest wymagana żadna dodatkowa gałąź dla potwierdzenia.

gnasher729
źródło
-2

Polecam całkowicie unikać innych . Użyj, ifaby zadeklarować, co powinien obsługiwać blok kodu, i zakończ blok, wychodząc z funkcji.

Powoduje to, że kod jest bardzo jasny:

setCircle(circle, i, { current })
{
    if (i == current)
    {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
        return
    }
    if (i < current)
    {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
        return
    }
    if (i > current)
    {
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
        return
    }
    throw new Exception("Condition not handled.");
}

Finał ifjest oczywiście zbędny ... dzisiaj. Bardzo ważna może być myśl, jeśli / kiedy jakiś przyszły programista zmieni aranżację bloków. Pomocne jest więc pozostawienie go tam.

John Wu
źródło
1
W rzeczywistości jest to bardzo niejasne, ponieważ teraz muszę rozważyć, czy wykonanie pierwszej gałęzi może zmienić wynik drugiego testu. Plus bezcelowe wielokrotne zwroty. Plus możliwość, że żaden warunek nie jest spełniony.
gnasher729,
Właściwie piszę, jeśli takie oświadczenia, gdy celowo oczekuję, że można wziąć wiele spraw. Jest to szczególnie przydatne w językach z instrukcjami przełączającymi, które nie pozwalają się przewrócić. Myślę, że jeśli wybory są wzajemnie wykluczające, należy to napisać, aby było oczywiste, że przypadki są wzajemnie wykluczające (przy użyciu innego). A jeśli nie jest tak napisane, to implikacja jest taka, że ​​przypadki nie wykluczają się wzajemnie (możliwe jest przewrócenie się).
Jerry Jeremiah
@ gnasher729 Całkowicie rozumiem twoją reakcję. Znam bardzo mądrych ludzi, którzy początkowo mieli problemy z „unikaniem innych” i „wczesnym powrotem”. Zachęcam do poświęcenia trochę czasu na przyzwyczajenie się do pomysłu i przyjrzenie się mu - ponieważ te pomysły obiektywnie i mierzalnie zmniejszają złożoność. Wczesny powrót faktycznie odnosi się do twojego pierwszego punktu (musisz rozważyć, czy gałąź może zmienić kolejny test). I „możliwość, że żaden warunek nie jest prawdziwy” nadal istnieje niezależnie; dzięki temu stylowi otrzymujesz oczywisty wyjątek, a nie ukryty błąd logiczny.
John Wu
Ale czy cykliczna złożoność obu stylów nie jest dokładnie taka sama? tzn. równa liczbie warunków
gaazkam