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

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();
    }
}

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.

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.

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.

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.

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);
}

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.