„Jak zły” jest niepowiązany kod w bloku try-catch-wreszcie?

11

Jest to powiązane pytanie: Czy użycie klauzuli w końcu do wykonywania pracy po powrocie jest złe / niebezpieczne?

W przywoływanym Q ostatecznie kod jest związany z zastosowaną strukturą i koniecznością pobierania z wyprzedzeniem. Moje pytanie jest trochę inne i uważam, że jest to istotne dla szerszej publiczności. Moim szczególnym przykładem jest aplikacja C # winform, ale w końcu dotyczyłaby również użycia C ++ / Java.

Zauważam sporo bloków typu try-catch-wreszcie, w których jest dużo kodu niezwiązanego z wyjątkami i obsługą / czyszczeniem wyjątków ukrytych w bloku. Przyznaję, że mam tendencję do posiadania bardzo ścisłych bloków try-catch-wreszcie z kodem ściśle związanym z wyjątkiem i obsługą. Oto kilka przykładów tego, co widzę.

W blokach try będzie wiele wstępnych wywołań i zmiennych, które prowadzą do kodu, który może zostać wygenerowany. Informacje o logowaniu zostaną skonfigurowane i uruchomione również w bloku try.

Na koniec bloki będą miały wywołania formatowania form / module / control (mimo że aplikacja ma się zakończyć, jak wyrażono w bloku catch), a także tworzyć nowe obiekty, takie jak panele.

W przybliżeniu:

    methodName (...)
    {
        próbować
        {
            // Dużo kodu dla metody ...
            // kod, który mógłby wyrzucić ...
            // Dużo więcej kodu dla metody i zwrot ...
        }
        złapać (coś)
        {// obsłuż wyjątek}
        Wreszcie
        {
            // niektóre porządki z powodu wyjątku, zamykanie rzeczy
            // więcej kodu dla rzeczy, które zostały utworzone (ignorując fakt, że mogły zostać zgłoszone wyjątki) ...
            // może stworzyć więcej obiektów
        }
    }

Kod działa, więc ma pewną wartość. Nie jest dobrze zamknięty, a logika jest nieco skomplikowana. Jestem (boleśnie) zaznajomiony z ryzykiem związanym z przesuwaniem kodu i refaktoryzacją, więc moje pytanie sprowadza się do chęci poznania doświadczeń innych osób z podobnie skonstruowanym kodem.

Czy zły styl uzasadnia dokonywanie zmian? Czy ktoś został poważnie poparzony w podobnej sytuacji? Czy chciałbyś podzielić się szczegółami tego złego doświadczenia? Zostaw to, bo jestem przesadnie reagujący i nie jest to taki zły styl? Zyskujesz korzyści związane z utrzymaniem porządku?

Społeczność
źródło
Tag „C ++” nie należy tutaj, ponieważ C ++ nie ma (i nie potrzebuje) finally. Wszystkie dobre zastosowania są objęte RAII / RRID / SBRM (dowolny akronim, jaki chcesz).
David Thornley,
2
@DavidThornley: Oprócz przykładu, w którym użyto słowa kluczowego „wreszcie”, reszta pytania doskonale pasuje do C ++. Jestem programistą C ++ i to słowo kluczowe tak naprawdę mnie nie myliło. Biorąc pod uwagę, że mam podobne rozmowy ze swoim zespołem na pół-regularne, to pytanie jest bardzo istotne dla tego, co robimy z C ++
DXM
@DXM: Mam problem z wizualizacją tego (i wiem, co finallyrobi w C #). Co to jest odpowiednik C ++? Myślę o kodzie po catch, i to samo dotyczy kodu po C # finally.
David Thornley
@DavidThornley: kod w końcu jest kodem, który wykonuje się bez względu na wszystko .
lub
1
@ nightcracker, to nie jest poprawne. Nie zostanie wykonane, jeśli to zrobisz Environment.FailFast(); może nie zostać wykonany, jeśli masz nieprzechwycony wyjątek. I staje się jeszcze bardziej skomplikowane, jeśli masz blok iteratora, z finallyktórym iterujesz ręcznie.
sick

Odpowiedzi:

10

Przeżyłem bardzo podobną sytuację, gdy miałem do czynienia ze strasznym, starszym kodem Windows Forms napisanym przez programistów, który wyraźnie nie wiedział, co robią.

Po pierwsze, nie przesadzasz. To jest zły kod. Jak powiedziałeś, blok catch powinien polegać na przerwaniu i przygotowaniu się do zatrzymania. Nie czas na tworzenie obiektów (szczególnie paneli). Nie mogę nawet zacząć wyjaśniać, dlaczego to jest złe.

Biorąc to pod uwagę ...

Moja pierwsza rada: jeśli nie jest zepsuta, nie dotykaj jej!

Jeśli Twoim zadaniem jest utrzymanie kodu, musisz starać się go nie łamać. Wiem, że to bolesne (byłem tam), ale musisz zrobić wszystko, aby nie złamać tego, co już działa.

Moja druga rada brzmi: jeśli musisz dodać więcej funkcji, spróbuj zachować istniejącą strukturę kodu tak bardzo, jak to możliwe, aby nie złamać kodu.

Przykład: jeśli istnieje obrzydliwe zdanie na temat zamiany, które Twoim zdaniem może zostać zastąpione odpowiednim dziedzictwem, musisz być ostrożny i przemyśleć dwa razy, zanim zdecydujesz się przenieść rzeczy.

Na pewno znajdziesz sytuacje, w których refaktoryzacja jest właściwym podejściem, ale uwaga: kod refaktoryzacji jest bardziej podatny na wprowadzanie błędów . Tę decyzję musisz podjąć z perspektywy właścicieli aplikacji, a nie dewelopera. Musisz więc pomyśleć, czy wysiłek (pieniądze) potrzebny do rozwiązania problemu jest wart refaktoryzacji, czy nie. Wielokrotnie widziałem programistę, który spędzał kilka dni naprawiając coś, co tak naprawdę nie jest zepsute tylko dlatego, że uważa, że ​​„kod jest brzydki”.

Moja trzecia rada brzmi: zostaniesz spalony, jeśli złamiesz kod, nie ma znaczenia, czy to twoja wina, czy nie.

Jeśli zostałeś zatrudniony do obsługi technicznej, tak naprawdę nie ma znaczenia, czy aplikacja się rozpada, ponieważ ktoś inny podjął złe decyzje. Z perspektywy użytkownika działało to wcześniej, a teraz go zepsułeś. Ci połamał go!

Joel bardzo dobrze pisze w swoim artykule, wyjaśniając kilka powodów, dla których nie powinieneś przepisywać starszego kodu.

http://www.joelonsoftware.com/articles/fog0000000069.html

Powinieneś czuć się naprawdę źle z tego rodzaju kodem (i nigdy nie powinieneś pisać czegoś takiego), ale utrzymywanie go to zupełnie inny potwór.

O moim doświadczeniu: musiałem utrzymywać kod przez około 1 rok i ostatecznie byłem w stanie przepisać go od zera, ale nie wszystkie naraz.

Stało się tak, że kod był tak zły, że nowych funkcji nie można było wdrożyć. Istniejąca aplikacja miała poważne problemy z wydajnością i użytecznością. W końcu poproszono mnie o zmianę, która zajęłaby mi 3-4 miesiące (głównie dlatego, że praca z tym kodem zajęła mi znacznie więcej czasu niż zwykle). Pomyślałem, że mogę przepisać cały ten utwór (w tym zaimplementować pożądaną nową funkcję) w ciągu około 5-6 miesięcy. Przedstawiłem tę propozycję interesariuszom, którzy zgadzają się ją przepisać (na szczęście dla mnie).

Po przepisaniu tego utworu zrozumieli, że mogę dostarczyć znacznie lepsze rzeczy niż to, co już mieli. Dzięki temu mogłem przepisać całą aplikację.

Moje podejście polegało na przepisaniu go kawałek po kawałku. Najpierw zastąpiłem cały interfejs użytkownika (Windows Forms), następnie zacząłem zastępować warstwę komunikacyjną (wywołania usługi sieci Web), a na koniec zastąpiłem całą implementację serwera (była to gruba aplikacja typu klient / serwer).

Kilka lat później ta aplikacja stała się pięknym kluczowym narzędziem używanym przez całą firmę. Jestem w 100% pewien, że nigdy nie byłoby to możliwe, gdybym nie napisał wszystkiego od nowa.

Mimo że udało mi się to zrobić, ważną częścią jest to, że zainteresowane strony zatwierdziły go i byłem w stanie przekonać ich, że było warte swojej ceny. Tak więc, podczas gdy musisz utrzymywać istniejącą aplikację, staraj się, aby niczego nie zepsuć, a jeśli jesteś w stanie przekonać właścicieli o korzyściach z przepisania, spróbuj zrobić to tak, jak Jack the Ripper: według kawałków.

Alex
źródło
1
+1. Ale ostatnie zdanie odnosi się do seryjnego zabójcy. Czy to naprawdę konieczne?
MarkJ
Podoba mi się część tego, ale jednocześnie pozostawianie zepsutych projektów na miejscu i dodawanie rzeczy często prowadzi do gorszego projektu. Lepiej jest dowiedzieć się, jak to naprawić i naprawić, chociaż oczywiście w wyważonym podejściu.
Ricky Clarkson,
@RickyClarkson Zgadzam się. Naprawdę trudno jest wiedzieć, kiedy przesadzasz (refaktoryzacja nie jest tak naprawdę konieczna) i kiedy naprawdę szkodzisz aplikacji, dodając zmiany.
Alex
@RickyClarkson Zgadzam się z tym tak bardzo, że mogłem nawet przepisać projekt, w który byłem zaangażowany. Ale przez długi czas musiałem ostrożnie dodawać rzeczy, starając się spowodować minimalne obrażenia. O ile prośba nie polega na naprawieniu czegoś, co jest główną przyczyną złej decyzji projektowej (co zwykle nie ma miejsca), powiedziałbym, że projekt aplikacji nie powinien zostać zmieniony.
Alex
@RickyClarkson To była część doświadczenia społeczności, z której chciałem skorzystać. Deklarowanie całego kodu jako złego jest równie złym anty-wzorem. Jednak w tym kodzie są pewne rzeczy, nad którymi pracuję, i tak naprawdę nie powinny być wykonywane jako część bloku w końcu. Informacje zwrotne i odpowiedzi Alexa były tym, o czym chciałem przeczytać.
2

Jeśli nie ma potrzeby zmiany kodu, pozostaw go bez zmian. Ale jeśli musisz zmienić kod, ponieważ musisz naprawić jakiś błąd lub zmienić funkcjonalność, będziesz musiał go zmienić, jeśli ci się spodoba, czy nie. IMHO jest często bezpieczniejsze i mniej podatne na błędy, jeśli najpierw przekształcisz go na mniejsze, lepiej zrozumiałe części, a następnie dodasz funkcjonalność. Gdy masz pod ręką narzędzia do automatycznego refaktoryzacji, można to zrobić stosunkowo bezpiecznie, z bardzo niewielkim ryzykiem wprowadzenia nowych błędów. I zalecam, abyś otrzymał kopię książki Michaela Feathersa

http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052

który da ci cenne wskazówki, jak uczynić kod bardziej testowalnym, dodaj testy jednostkowe i unikaj łamania kodu podczas dodawania nowych funkcji.

Jeśli zrobisz to dobrze, twoja metoda stanie się na tyle prosta, że ​​try-catch-wreszcie nie będzie już zawierał niepowiązanego kodu w niewłaściwych miejscach.

Doktor Brown
źródło
2

Załóżmy, że masz sześć metod wywoływania, z których cztery należy wywołać tylko wtedy, gdy pierwsza zakończy się bez błędu. Rozważ dwie alternatywy:

try {
     method1();  // throws
     method2();
     method3();
     method4();
     method5();
 } catch(e) {
     // handle error
 }
 method6();

vs

 try {
      method1();  // throws
 }
 catch(e) {
      // handle error
 }
 if(! error ) {
     method2();
     method3();
     method4();
     method5();
 }
 method6();

W drugim kodzie traktujesz wyjątki jak kody powrotu. Koncepcyjnie jest to identyczne z:

rc = method1();
if( rc != error ) {
     method2();
     method3();
     method4();
     method5();
 }
 method6();

Jeśli zamierzamy zawinąć każdą metodę, która może zgłosić wyjątek w bloku try / catch, to po co w ogóle mieć wyjątki w funkcji języka? Nie ma korzyści i jest więcej pisania.

Powodem, dla którego mamy wyjątki w językach, jest to, że w starych, złych czasach kodów powrotu często mieliśmy sytuacje powyżej, w których mieliśmy wiele metod, które mogły zwracać błędy, a każdy błąd oznaczał całkowite przerwanie wszystkich metod. Na początku mojej kariery, w dawnych czasach C, wszędzie widziałem taki kod:

rc = method1();
if( rc != error ) {
     rc = method2();
     if( rc != error ) {
         rc = method3();
         if( rc != error ) {
             rc = method4();
             if(rc != error ) {
                 method5();
             }
         }
     }
 }
 method6();

Był to niezwykle powszechny wzorzec, który wyjątkowo starannie rozwiązał wyjątki, umożliwiając powrót do pierwszego przykładu powyżej. Reguła stylu, która mówi, że każda metoda ma własny blok wyjątków, całkowicie wyrzuca to przez okno. Dlaczego więc zawracasz sobie głowę?

Zawsze powinieneś starać się, aby blok try zastępował zestaw kodu, który jest koncepcyjnie jednostką. Powinien otaczać kod, dla którego jeśli w jednostce kodowej wystąpił błąd, nie ma sensu uruchamiać żadnego innego kodu w jednostce kodowej.

Wracając do pierwotnego celu wyjątków: pamiętaj, że zostały one utworzone, ponieważ często kod nie jest w stanie łatwo poradzić sobie z błędami w chwili ich wystąpienia. Wyjątkiem jest umożliwienie radzenia sobie z błędami znacznie później w kodzie lub w górę stosu. Ludzie zapominają o tym i w wielu przypadkach łapią je lokalnie, gdy lepiej byłoby po prostu udokumentować, że dana metoda może zgłosić ten wyjątek. Na świecie jest za dużo kodu, który wygląda następująco:

void method0() : throws MyNewException 
{
    try {
        method1();  // throws MyOtherException
    }
    catch(e) {
        if(e == MyOtherException)
            throw MyNewException();
    }
    method2();
}

Tutaj dodajesz tylko warstwy, a warstwy wprowadzają zamieszanie. Po prostu zrób to:

void method0() : throws MyOtherException
{
    method1();
    method2();
}

Poza tym czuję, że jeśli zastosujesz się do tej zasady i znajdziesz więcej niż kilka bloków try / catch w metodzie, powinieneś zapytać, czy sama metoda jest zbyt złożona i czy należy ją podzielić na wiele metod .

Na wynos: blok try powinien zawierać zestaw kodu, który należy przerwać, jeśli w dowolnym miejscu bloku wystąpi błąd. To, czy ten zestaw kodu ma jedną linię, czy tysiąc linii, nie ma znaczenia. (Chociaż jeśli masz tysiąc linii w metodzie, masz inne problemy).

Gort the Robot
źródło
Steven - dziękuję za dobrze ułożoną odpowiedź i całkowicie zgadzam się z twoimi przemyśleniami. Nie mam problemu z rozsądnie pokrewnym lub sekwencyjnym kodem typu żyjącym w jednym bloku try. Gdybym był w stanie opublikować fragment kodu, pokazałby 5–10 całkowicie niepowiązanych połączeń przed pierwszym wywołaniem możliwym do rzucenia. Jeden konkretny blok w końcu był znacznie gorszy - oderwano kilka nowych wątków i wywołano dodatkowe, nieoczyszczające procedury. I jeśli chodzi o tę kwestię, wszystkie połączenia w tym wreszcie bloku były niezwiązane z niczym, co mogło zostać rzucone.
Jednym z głównych problemów związanych z tym podejściem jest to, że nie wprowadza rozróżnienia między przypadkami, w których method0można by oczekiwać, że zostanie zgłoszony wyjątek, a tymi, w których nie ma takiej możliwości. Załóżmy na przykład, że method1czasami może wyrzucić InvalidArgumentException, ale jeśli nie, spowoduje to, że obiekt przejdzie w tymczasowo niepoprawny stan, który zostanie poprawiony przez method2, co powinno zawsze przywracać obiekt do prawidłowego stanu. Jeśli metoda2 wyrzuci InvalidArgumentException, kod, który spodziewa się złapać taki wyjątek method1, go złapie, nawet jeśli ...
supercat
... stan systemu będzie zgodny z oczekiwaniami kodu. Jeśli wyjątek zgłoszony w metodzie 1 powinien być traktowany inaczej niż wyjątek zgłoszony w metodzie 2, to jak można to rozsądnie osiągnąć bez owijania go?
supercat
Idealnie twoje wyjątki są na tyle szczegółowe, że jest to rzadka sytuacja.
Gort the Robot