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();
}
}
java
programming-practices
Sok Pomaranczowy
źródło
źródło
rethrow
w rzeczywistości niethrow
stanowi wyjątku. (co może się zdarzyć, jeśli implementacja ulegnie zmianie)Odpowiedzi:
Po pierwsze, dziękuję za udawanie pytania i pokazanie nam, co
rethrow
robi. 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/catch
bloku. 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:Następnie,
throw
wyraźnie, a twój kompilator jest szczęśliwy: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ńculogAndWrap
, 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ł,throw
jest 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ść
rethrow
można również zapisać jako funkcję, ale mam problemy z jej bieżącą implementacją:Istnieje wiele takich bezużytecznych rzutówOdlewy są w rzeczywistości wymagane (patrz komentarze):Podczas rzucania / zwracania nowych wyjątków stary jest odrzucany; w poniższym kodzie a
MailLoginNotAvailableException
nie pozwala mi dowiedzieć się, który login jest niedostępny, co jest niewygodne; ponadto stacktraces będą niekompletne:Dlaczego kod źródłowy nie rzuca tych specjalnych wyjątków w pierwszej kolejności? Podejrzewam, że
rethrow
jest 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ęciacatch
irethrow
kod tutaj bez większych refaktoryzacji.źródło
logAndWrap
metodzie 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óbThrowables.propagate
wdraża się Guava .logAndWrap
ale 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.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
retrieveUserMailConfiguration
funkcji, w którym to momencie błąd nie mareturn
instrukcji.RuntimeException
Rzucony nie ma złagodzić ten problem kompilatora, ale jest to raczej niezdarny sposób to zrobić. Innym sposobem uniknięciafunction must return a value
błędu jest dodaniereturn null; //to keep the compiler happy
instrukcji, ale moim zdaniem jest to równie niezręczne.Więc osobiście zastąpiłbym to:
z tym:
lub jeszcze lepiej (zgodnie z sugestią Coredump ):
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.
źródło
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ą.throw reportAndTransform(e)
. Wiele wzorów projektowych to łaty, brakuje funkcji językowych. To jest jeden z nich.throw
Prawdopodobnie 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 niereturn
jest to konieczne pothrow
, ale nie po niestandardowejrethrow()
metody, a nie ma@NoReturn
adnotacji ż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.źródło
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ź.rethrow()
metoda ukrywa fakt, żecatch
ponownie 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.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 maszreturn null
teraznull
w aplikacji zmienną wartość, która nie była oczekiwana. Kto wie, gdzie we wnioskuNullPointerException
powstanie? 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.return null
prawdopodobnie maskuje błąd, ponieważ nie można odróżnić innych przypadków, w których wracanull
z tego błędu.The
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.źródło
Nie wiem czy istnieje konwencja.
W każdym razie inną sztuczką byłoby zrobić tak:
Umożliwiając to:
I żadne sztuczne nie
RuntimeException
jest już potrzebne. Chociażrethrow
tak 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
RuntimeException
lub 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ę
rethrow
i mieć coś takiego:źródło
Jeśli
try
całkowicie usuniesz blok, nie potrzebujeszrethrow
anithrow
. Ten kod robi dokładnie to samo co oryginał: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ąteke
. Jeśli tarethrow
metoda faktycznie robi coś innego niż ponowne zgłoszenie wyjątku, wówczas pozbycie się go zmienia semantykę tej metody.źródło
catch(Exception)
którym jest wstrętny zapach kodu.rethrow
jest 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. Wrethrow
funkcji niestandardowej występują efekty uboczne .