Czy zgłaszanie nowych wyjątków RuntimeException w nieosiągalnym kodzie jest złym stylem?

31

Zostałem przydzielony do prowadzenia aplikacji napisanej jakiś czas temu przez bardziej wykwalifikowanych programistów. Natknąłem się na ten fragment kodu:

public Configuration retrieveUserMailConfiguration(Long id) throws MailException {
        try {
            return translate(mailManagementService.retrieveUserMailConfiguration(id));
        } catch (Exception e) {
            rethrow(e);
        }
        throw new RuntimeException("cannot reach here");
    }

Jestem ciekawy, czy rzucanie RuntimeException("cannot reach here")jest uzasadnione. Prawdopodobnie brakuje mi czegoś oczywistego, wiedząc, że ten fragment kodu pochodzi od bardziej doświadczonego kolegi.
EDYCJA: Oto przerzucanie treści, do których odnosiły się niektóre odpowiedzi. Uznałem to za nieistotne w tym pytaniu.

private void rethrow(Exception e) throws MailException {
        if (e instanceof InvalidDataException) {
            InvalidDataException ex = (InvalidDataException) e;
            rethrow(ex);
        }
        if (e instanceof EntityAlreadyExistsException) {
            EntityAlreadyExistsException ex = (EntityAlreadyExistsException) e;
            rethrow(ex);
        }
        if (e instanceof EntityNotFoundException) {
            EntityNotFoundException ex = (EntityNotFoundException) e;
            rethrow(ex);
        }
        if (e instanceof NoPermissionException) {
            NoPermissionException ex = (NoPermissionException) e;
            rethrow(ex);
        }
        if (e instanceof ServiceUnavailableException) {
            ServiceUnavailableException ex = (ServiceUnavailableException) e;
            rethrow(ex);
        }
        LOG.error("internal error, original exception", e);
        throw new MailUnexpectedException();
    }


private void rethrow(ServiceUnavailableException e) throws
            MailServiceUnavailableException {
        throw new MailServiceUnavailableException();
    }

private void rethrow(NoPermissionException e) throws PersonNotAuthorizedException {
    throw new PersonNotAuthorizedException();
}

private void rethrow(InvalidDataException e) throws
        MailInvalidIdException, MailLoginNotAvailableException,
        MailInvalidLoginException, MailInvalidPasswordException,
        MailInvalidEmailException {
    switch (e.getDetail()) {
        case ID_INVALID:
            throw new MailInvalidIdException();
        case LOGIN_INVALID:
            throw new MailInvalidLoginException();
        case LOGIN_NOT_ALLOWED:
            throw new MailLoginNotAvailableException();
        case PASSWORD_INVALID:
            throw new MailInvalidPasswordException();
        case EMAIL_INVALID:
            throw new MailInvalidEmailException();
    }
}

private void rethrow(EntityAlreadyExistsException e)
        throws MailLoginNotAvailableException, MailEmailAddressAlreadyForwardedToException {
    switch (e.getDetail()) {
        case LOGIN_ALREADY_TAKEN:
            throw new MailLoginNotAvailableException();
        case EMAIL_ADDRESS_ALREADY_FORWARDED_TO:
            throw new MailEmailAddressAlreadyForwardedToException();
    }
}

private void rethrow(EntityNotFoundException e) throws
        MailAccountNotCreatedException,
        MailAliasNotCreatedException {
    switch (e.getDetail()) {
        case ACCOUNT_NOT_FOUND:
            throw new MailAccountNotCreatedException();
        case ALIAS_NOT_FOUND:
            throw new MailAliasNotCreatedException();
    }
}
Sok Pomaranczowy
źródło
12
jest technicznie osiągalny, jeśli rethroww rzeczywistości nie throwstanowi wyjątku. (co może się zdarzyć, jeśli implementacja ulegnie zmianie)
njzk2
34
Na marginesie, AssertionError może być semantycznie lepszym wyborem niż RuntimeException.
JvR
1
Ostatni rzut jest zbędny. Jeśli włączysz findbugs lub inne narzędzia analizy statycznej, oznaczałoby to usunięcie tego typu linii.
Bon Ami
7
Teraz, gdy widzę resztę kodu, myślę, że może powinieneś zmienić „bardziej wykwalifikowanych programistów” na „więcej starszych programistów”. Przegląd kodu chętnie wyjaśni dlaczego.
JvR
3
Próbowałem stworzyć hipotetyczne przykłady, dlaczego wyjątki są okropne. Ale nic, co kiedykolwiek zrobiłem, nie podtrzymuje świecy.
Paul Draper,

Odpowiedzi:

36

Po pierwsze, dziękuję za udawanie pytania i pokazanie nam, co rethrowrobi. Tak więc w rzeczywistości przekształca wyjątki o właściwościach w bardziej wyjątkowe klasy wyjątków. Więcej na ten temat później.

Ponieważ tak naprawdę nie odpowiedziałem pierwotnie na główne pytanie, oto on: tak, generalnie złym stylem jest wprowadzanie wyjątków czasu wykonywania w nieosiągalnym kodzie; lepiej użyj twierdzeń, a jeszcze lepiej uniknij problemu. Jak już wspomniano, tutaj kompilator nie może mieć pewności, że kod nigdy nie wychodzi z try/catchbloku. Możesz refaktoryzować kod, korzystając z tego, że ...

Błędy to wartości

(Nic dziwnego, że jest dobrze znany w go )

Użyjmy prostszego przykładu, tego, którego użyłem przed edycją: wyobraź sobie, że coś logujesz i budujesz wyjątek otoki jak w odpowiedzi Konrada . Nazwijmy to logAndWrap.

Zamiast rzucać wyjątek jako efekt uboczny logAndWrap, możesz pozwolić mu działać jako efekt uboczny i sprawić, że zwróci wyjątek (przynajmniej ten podany na wejściu). Nie musisz używać ogólnych, tylko podstawowe funkcje:

private Exception logAndWrap(Exception exception) {
    // or whatever it actually does
    Log.e("Ouch! " + exception.getMessage());
    return new CustomWrapperException(exception);
}

Następnie, throwwyraźnie, a twój kompilator jest szczęśliwy:

try {
     return translate(mailManagementService.retrieveUserMailConfiguration(id));
} catch (Exception e) {
     throw logAndWrap(e);
}

Co jeśli zapomnisz throw?

Jak wyjaśniono w komentarzu Joe23 , defensywny sposób programowania zapewniający, że wyjątek jest zawsze zgłaszany, polegałby na jawnym wykonaniu throw new CustomWrapperException(exception)na końcu logAndWrap, tak jak to robi Guava.Throwables . W ten sposób wiesz, że wyjątek zostanie zgłoszony, a analizator typów będzie zadowolony. Jednak niestandardowe wyjątki muszą być odznaczone, co nie zawsze jest możliwe. Oceniłbym również, że ryzyko, że programista nie napisał, throwjest bardzo niskie: programista musi o tym zapomnieć, a otaczająca go metoda nie powinna niczego zwracać, w przeciwnym razie kompilator wykryłby brakujący zwrot. Jest to interesujący sposób na walkę z systemem typów i działa jednak.

Rethrow

Rzeczywistość rethrowmożna również zapisać jako funkcję, ale mam problemy z jej bieżącą implementacją:

  • Istnieje wiele takich bezużytecznych rzutów Odlewy są w rzeczywistości wymagane (patrz komentarze):

    if (e instanceof ServiceUnavailableException) {
        ServiceUnavailableException ex = (ServiceUnavailableException) e;
        rethrow(ex);
    }
  • Podczas rzucania / zwracania nowych wyjątków stary jest odrzucany; w poniższym kodzie a MailLoginNotAvailableExceptionnie pozwala mi dowiedzieć się, który login jest niedostępny, co jest niewygodne; ponadto stacktraces będą niekompletne:

    private void rethrow(EntityAlreadyExistsException e)
        throws MailLoginNotAvailableException, MailEmailAddressAlreadyForwardedToException {
        switch (e.getDetail()) {
            case LOGIN_ALREADY_TAKEN:
                throw new MailLoginNotAvailableException();
            case EMAIL_ADDRESS_ALREADY_FORWARDED_TO:
                throw new MailEmailAddressAlreadyForwardedToException();
        }
    }
  • Dlaczego kod źródłowy nie rzuca tych specjalnych wyjątków w pierwszej kolejności? Podejrzewam, że rethrowjest używana jako warstwa zgodności między podsystemem (mailingowym) a logiką biznesową (być może chodzi o to, aby ukryć szczegóły implementacji, takie jak zgłoszone wyjątki, zastępując je niestandardowymi wyjątkami). Nawet jeśli zgadzam się, że byłoby lepiej mieć catch-wolny kod, jak zasugerowano w odpowiedzi Pete Beckera , nie sądzę, będziesz miał możliwość usunięcia catchi rethrowkod tutaj bez większych refaktoryzacji.

rdzeń rdzeniowy
źródło
1
Obsady nie są bezużyteczne. Są one wymagane, ponieważ nie ma dynamicznej wysyłki w zależności od typu argumentu. Typ argumentu musi być znany w czasie kompilacji, aby wywołać odpowiednią metodę.
Joe23
W logAndWrapmetodzie możesz zgłosić wyjątek zamiast go zwracać, ale zachowaj typ zwracany (Runtime)Exception. W ten sposób blok catch może pozostać taki, jak napisałeś (bez wrzucania nieosiągalnego kodu). Ale ma dodatkową zaletę, której nie można zapomnieć throw. Jeśli zwrócisz wyjątek i zapomnisz rzut, spowoduje to dyskretne połknięcie wyjątku. Rzucanie z typem zwracanym RuntimeException sprawi, że kompilator będzie szczęśliwy, a jednocześnie uniknie tych błędów. W ten sposób Throwables.propagatewdraża się Guava .
Joe23
@ Joe23 Dziękujemy za wyjaśnienie dotyczące wysyłki statycznej. O rzucaniu wyjątków wewnątrz funkcji: czy masz na myśli, że opisany możliwy błąd to blok catch, który wywołuje, logAndWrapale nic nie robi ze zwróconym wyjątkiem? To interesujące. Wewnątrz funkcji, która musi zwrócić wartość, kompilator zauważy, że istnieje ścieżka bez zwracanej wartości, ale jest to przydatne w przypadku metod, które niczego nie zwracają. Dzięki jeszcze raz.
coredump
1
Właśnie o tym myślę. I tylko po to, aby podkreślić tę kwestię: nie jest to coś, co wymyśliłem i przypisałem sobie, ale zebrałem ze źródła Guava.
Joe23
@ Joe23 Dodałem wyraźne odniesienie do biblioteki
rdzeń
52

Ta rethrow(e);funkcja narusza zasadę mówiącą, że w normalnych okolicznościach funkcja powróci, podczas gdy w wyjątkowych okolicznościach funkcja zgłosi wyjątek. Ta funkcja narusza tę zasadę, zgłaszając wyjątek w normalnych okolicznościach. To jest źródłem całego zamieszania.

Kompilator zakłada, że ​​funkcja ta powróci w normalnych okolicznościach, o ile kompilator może to stwierdzić, wykonanie może osiągnąć koniec retrieveUserMailConfigurationfunkcji, w którym to momencie błąd nie ma returninstrukcji. RuntimeExceptionRzucony nie ma złagodzić ten problem kompilatora, ale jest to raczej niezdarny sposób to zrobić. Innym sposobem uniknięcia function must return a valuebłędu jest dodanie return null; //to keep the compiler happyinstrukcji, ale moim zdaniem jest to równie niezręczne.

Więc osobiście zastąpiłbym to:

rethrow(e);

z tym:

report(e); 
throw e;

lub jeszcze lepiej (zgodnie z sugestią Coredump ):

throw reportAndTransform(e);

W ten sposób przepływ kontroli stałby się oczywisty dla kompilatora, tak więc twoja wersja ostateczna throw new RuntimeException("cannot reach here");stałaby się nie tylko redundantna, ale nawet niemożliwa do kompilacji, ponieważ byłaby oznaczona przez kompilator jako kod nieosiągalny.

To najbardziej elegancki i właściwie najprostszy sposób na wyjście z tej brzydkiej sytuacji.

Mike Nakis
źródło
1
-1 Sugerowanie podzielenia funkcji na dwie instrukcje może wprowadzić błędy programowania z powodu złego kopiowania / wklejania; Jestem również nieco zaniepokojony faktem, że edytowana pytanie, które sugerowałyby, throw reportAndTransform(e)a kilka minut po tym, jak pisał własną odpowiedź. Być może zmodyfikowałeś swoje pytanie niezależnie od tego i przepraszam z góry, jeśli tak jest, ale w przeciwnym razie przyznanie odrobiny uznania jest czymś, co ludzie zwykle robią.
coredump
4
@coredump W rzeczywistości przeczytałem twoją sugestię, a nawet przeczytałem inną odpowiedź, która przypisuje ci tę sugestię, i przypomniałem sobie, że faktycznie stosowałem właśnie taki konstrukt w przeszłości, więc postanowiłem dołączyć go do mojej odpowiedzi. Pomyślałem, że nie jest to tak ważne, aby uwzględnić atrybucję, ponieważ nie mówimy tutaj o oryginalnym pomyśle, ale jeśli masz z tym problem, nie chcę cię denerwować, więc przypisanie jest już dostępne, w komplecie z linkiem. Twoje zdrowie.
Mike Nakis,
Nie chodzi o to, że mam patent na ten konstrukt, który jest dość powszechny; ale wyobraź sobie, że piszesz odpowiedź i niedługo potem jest „zasymilowana” przez inną. Tak więc uznanie autorstwa jest bardzo mile widziane. Dziękuję Ci.
rdzeń rdzeniowy
3
Nie ma nic złego per se z funkcją, która nie zwraca wartości. Zdarza się to cały czas, na przykład w systemach czasu rzeczywistego. Ostatecznym problemem jest wada java polegająca na tym, że język analizuje i egzekwuje koncepcję poprawności języków, a mimo to język nie zapewnia mechanizmu adnotacji, że funkcja nie powraca. Istnieją jednak wzorce projektowe, które omijają ten problem, takie jak throw reportAndTransform(e). Wiele wzorów projektowych to łaty, brakuje funkcji językowych. To jest jeden z nich.
David Hammen,
2
Z drugiej strony wiele współczesnych języków jest wystarczająco skomplikowanych. Przekształcenie każdego wzorca projektu w funkcję języka podstawowego jeszcze bardziej go rozdęci. To dobry przykład wzorca projektowego, który nie należy do podstawowego języka. Jak napisano, jest dobrze zrozumiany nie tylko przez kompilator, ale także przez wiele innych narzędzi, które muszą parsować kod.
MSalters
7

throwPrawdopodobnie dodane obejść „metody musi zwracać wartość” błąd, który w przeciwnym razie występuje - dane Java płynąć analizator jest wystarczająco inteligentny, aby zrozumieć, że nie returnjest to konieczne po throw, ale nie po niestandardowej rethrow()metody, a nie ma @NoReturnadnotacji że możesz to naprawić.

Niemniej jednak tworzenie nowego wyjątku w nieosiągalnym kodzie wydaje się zbędne. Po prostu pisałbym return null, wiedząc, że tak naprawdę nigdy się nie wydarzy.

Kilian Foth
źródło
Masz rację. Sprawdziłem, co by się stało, jeśli skomentuję throw new...wiersz i narzeka na brak oświadczenia zwrotnego. Czy to złe programowanie? Czy istnieje jakaś konwencja dotycząca zastępowania nieosiągalnego wyciągu zwrotnego? Pozostawię to pytanie na chwilę bez odpowiedzi, aby zachęcić do większego wkładu. Niemniej jednak dziękuję za odpowiedź.
Sok Pomaranczowy
3
@SokPomaranczowy Tak, to złe programowanie. Ta rethrow()metoda ukrywa fakt, że catchponownie wprowadza wyjątek. Unikałbym robienia czegoś takiego. Ponadto, oczywiście, rethrow()może nie powrócić do wyjątku, w którym to przypadku dzieje się coś innego.
Ross Patterson
26
Proszę nie zamieniać wyjątku na return null. Z wyjątkiem tego, jeśli ktoś w przyszłości złamie kod, więc ostatnia linijka, w jaki sposób staje się osiągalna, będzie natychmiast jasne, kiedy wyjątek zostanie zgłoszony. Jeśli zamiast tego masz return nullteraz nullw aplikacji zmienną wartość, która nie była oczekiwana. Kto wie, gdzie we wniosku NullPointerExceptionpowstanie? Jeśli nie będziesz miał szczęścia i nie nastąpi to bezpośrednio po wywołaniu tej funkcji, bardzo trudno będzie znaleźć prawdziwy problem.
unholysampler
5
@MatthewRead Wykonanie kodu, który ma być nieosiągalny, jest poważnym błędem, return nullprawdopodobnie maskuje błąd, ponieważ nie można odróżnić innych przypadków, w których wraca nullz tego błędu.
CodesInChaos
4
Ponadto, jeśli funkcja w niektórych okolicznościach już zwraca wartość null, a w takich okolicznościach użyjesz również wartości null, użytkownicy mają teraz dwa możliwe scenariusze, w których mogą wystąpić, gdy otrzymają wartość null. Czasami to nie ma znaczenia, ponieważ powrót do wartości zerowej oznacza już „nie ma obiektu i nie precyzuję dlaczego”, więc dodanie jeszcze jednego możliwego powodu, dla którego nie ma znaczenia. Często jednak dodawanie dwuznaczności jest złe i na pewno oznacza, że ​​błędy będą początkowo traktowane jak „pewne okoliczności”, a nie od razu wyglądać jak „to jest zepsute”. Niepowodzenie wcześnie.
Steve Jessop
6

The

throw new RuntimeException("cannot reach here");

instrukcja wyjaśnia OSOBIE czytającej kod, co się dzieje, więc jest o wiele lepsza niż na przykład zwracanie wartości null. Ułatwia także debugowanie, jeśli kod zostanie zmieniony w nieoczekiwany sposób.

Jednak rethrow(e)wydaje się po prostu źle! Więc w twoim przypadku myślę, że refaktoryzacja kodu jest lepszą opcją. Zobacz inne odpowiedzi (podoba mi się to, co najlepsze w rdzeniu rdzeniowym), aby dowiedzieć się, jak uporządkować kod.

Ian
źródło
4

Nie wiem czy istnieje konwencja.

W każdym razie inną sztuczką byłoby zrobić tak:

private <T> T rethrow(Exception exception) {
    // or whatever it actually does
    Log.e("Ouch! " + exception.getMessage());
    throw new CustomWrapperException(exception);
}

Umożliwiając to:

try {
     return translate(mailManagementService.retrieveUserMailConfiguration(id));
} catch (Exception e) {
     return rethrow(e);
}

I żadne sztuczne nie RuntimeExceptionjest już potrzebne. Chociaż rethrowtak naprawdę nigdy nie zwraca żadnej wartości, jest teraz wystarczająca dla kompilatora.

Zwraca wartość w teorii (podpis metody), a następnie jest zwolniony z tego, ponieważ generuje wyjątek.

Tak, może to wyglądać dziwnie, ale z drugiej strony - rzucanie fantomem RuntimeExceptionlub zwracanie wartości zerowych, których nigdy nie będzie można zobaczyć na tym świecie - nie jest to również kwestia piękna.

Dążenie do czytelności, można zmienić nazwę rethrowi mieć coś takiego:

} catch (Exception e) {
     return nothingJustRethrow(e);
}
Konrad Morawski
źródło
2

Jeśli trycałkowicie usuniesz blok, nie potrzebujesz rethrowani throw. Ten kod robi dokładnie to samo co oryginał:

public Configuration retrieveUserMailConfiguration(Long id) throws MailException {
    return translate(mailManagementService.retrieveUserMailConfiguration(id));
}

Nie daj się zwieść faktowi, że pochodzi od bardziej doświadczonych programistów. To jest zgnilizna kodu i zdarza się to cały czas. Po prostu to napraw.

EDYCJA: Źle przeczytałem, rethrow(e)po prostu ponownie rzucając wyjątek e. Jeśli ta rethrowmetoda faktycznie robi coś innego niż ponowne zgłoszenie wyjątku, wówczas pozbycie się go zmienia semantykę tej metody.

Pete Becker
źródło
Ludzie nadal będą wierzyć, że łapania symboli wieloznacznych, które nic nie obsługują, są tak naprawdę programami obsługi wyjątków. Twoja wersja ignoruje błędne założenie, nie udając, że radzi sobie z tym, czego nie potrafi. Program wywołujący retrieveUserMailConfiguration () oczekuje, że coś odzyska. Jeśli ta funkcja nie może zwrócić czegoś sensownego, powinna zawieść szybko i głośno. To tak, jakby inne odpowiedzi nie widzi problemu z catch(Exception)którym jest wstrętny zapach kodu.
msw
1
@msw - Kodowanie kultu.
Pete Becker
@msw - z drugiej strony daje ci miejsce do ustawienia punktu przerwania.
Pete Becker
1
To tak, jakby nie było problemu z odrzuceniem całej logiki. Twoja wersja wyraźnie nie robi tego samego, co oryginał, który, nawiasem mówiąc, już zawodzi szybko i głośno. Wolałbym pisać metody bezłapowe, takie jak ta w twojej odpowiedzi, ale pracując z istniejącym kodem, nie powinniśmy zrywać umów z innymi działającymi częściami. Może się zdarzyć, że to, co jest zrobione, rethrowjest bezużyteczne (i nie o to tutaj chodzi), ale nie możesz po prostu pozbyć się kodu, który ci się nie podoba i powiedzieć, że jest równoważny z oryginałem, gdy najwyraźniej nie jest. W rethrowfunkcji niestandardowej występują efekty uboczne .
coredump
1
@ jpmc26 Sedno pytania dotyczy wyjątku „nie można tu dotrzeć”, który może wynikać z innych powodów oprócz poprzedniego bloku try / catch. Ale zgadzam się, że blok pachnie podejrzanie, a jeśli ta odpowiedź byłaby oryginalniej sformułowana, zyskałaby o wiele więcej głosów pozytywnych (nie głosowałem, btw). Ostatnia edycja jest dobra.
coredump