Ist das Auslösen neuer RuntimeExceptions in nicht erreichbarem Code ein schlechter Stil?

Ich wurde beauftragt, eine Anwendung zu warten, die vor einiger Zeit von erfahreneren Entwicklern geschrieben wurde. Ich bin auf diesen Code gestoßen:

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

Ich bin gespannt, ob das Werfen RuntimeException("cannot reach here")gerechtfertigt ist. Mir fehlt wahrscheinlich etwas Offensichtliches, da ich weiß, dass dieser Code von einem erfahreneren Kollegen stammt.
EDIT: Hier ist ein Rethrow-Body, auf den sich einige Antworten beziehen. Ich hielt es für unwichtig in dieser Frage.

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

Zunächst einmal vielen Dank, dass Sie Ihre Frage beantwortet und uns gezeigt haben, was zu tun rethrowist. Tatsächlich konvertiert es also Ausnahmen mit Eigenschaften in feinkörnigere Klassen von Ausnahmen. Dazu später mehr.

Da ich die Hauptfrage ursprünglich nicht wirklich beantwortet habe, heißt es hier: Ja, es ist im Allgemeinen ein schlechter Stil, Laufzeitausnahmen in nicht erreichbarem Code auszulösen. Verwenden Sie besser Behauptungen oder vermeiden Sie das Problem. Wie bereits erwähnt, kann der Compiler hier nicht sicher sein, dass der Code niemals den try/catchBlock verlässt. Sie können Ihren Code umgestalten, indem Sie den Vorteil nutzen, dass ...

Fehler sind Werte

^{( Es ist nicht überraschend, dass es in go bekannt ist )}

Nehmen wir ein einfacheres Beispiel, das ich vor Ihrer Bearbeitung verwendet habe: Stellen Sie sich vor, Sie protokollieren etwas und erstellen eine Wrapper-Ausnahme wie in Konrads Antwort . Nennen wir es logAndWrap.

Anstatt die Ausnahme als Nebeneffekt von logAndWrapauszulösen, können Sie sie als Nebeneffekt wirken lassen und eine Ausnahme zurückgeben (zumindest die in der Eingabe angegebene). Sie müssen keine Generika verwenden, sondern nur Grundfunktionen:

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

Dann throwexplizit Sie und Ihr Compiler freut sich:

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

Was ist, wenn Sie es vergessen throw?

Wie in Joe23s Kommentar erläutert , besteht eine Möglichkeit zur defensiven Programmierung , um sicherzustellen, dass die Ausnahme immer ausgelöst wird, darin, explizit throw new CustomWrapperException(exception)am Ende von ein zu machen logAndWrap, wie es von Guava.Throwables getan wird . Auf diese Weise wissen Sie, dass die Ausnahme ausgelöst wird, und Ihr Typanalysator ist glücklich. Ihre benutzerdefinierten Ausnahmen müssen jedoch deaktiviert sein, was nicht immer möglich ist. Außerdem würde ich das Risiko, dass ein throwEntwickler zum Schreiben fehlt, als sehr gering einstufen : Der Entwickler muss es vergessen und die umgebende Methode sollte nichts zurückgeben, da der Compiler sonst einen fehlenden Return feststellen würde. Dies ist eine interessante Möglichkeit, das Typensystem zu bekämpfen, und sie funktioniert trotzdem.

Erneut werfen

Das eigentliche rethrowkann auch als Funktion geschrieben werden, aber ich habe Probleme mit der aktuellen Implementierung:

• Es gibt viele nutzlose Darsteller wie Casts sind in der Tat erforderlich (siehe Kommentare):

  if (e instanceof ServiceUnavailableException) {
      ServiceUnavailableException ex = (ServiceUnavailableException) e;
      rethrow(ex);
  }

• Wenn neue Ausnahmen geworfen / zurückgegeben werden, wird die alte verworfen. Im folgenden Code kann MailLoginNotAvailableExceptionich mit a nicht erkennen, welche Anmeldung nicht verfügbar ist, was unpraktisch ist. Darüber hinaus sind Stacktraces unvollständig:

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

• Warum löst der Ursprungscode diese speziellen Ausnahmen überhaupt nicht aus? Ich vermute, dass dies rethrowals Kompatibilitätsebene zwischen einem (Mailing-) Subsystem und der Geschäftslogik verwendet wird (möglicherweise besteht die Absicht darin, Implementierungsdetails wie ausgelöste Ausnahmen zu verbergen, indem sie durch benutzerdefinierte Ausnahmen ersetzt werden). Auch wenn ich zustimme, dass es besser wäre, fangfreien Code zu haben, wie in Pete Beckers Antwort vorgeschlagen , glaube ich nicht, dass Sie die Möglichkeit haben werden, den Code catchund rethrowhier ohne größere Umgestaltungen zu entfernen .

Diese rethrow(e);Funktion verstößt gegen das Prinzip, dass unter normalen Umständen eine Funktion zurückgegeben wird, während unter außergewöhnlichen Umständen eine Funktion eine Ausnahme auslöst. Diese Funktion verletzt dieses Prinzip, indem sie unter normalen Umständen eine Ausnahme auslöst. Das ist die Quelle aller Verwirrung.

Der Compiler geht davon aus, dass diese Funktion unter normalen Umständen zurückgegeben wird, so weit der Compiler erkennen kann, dass die Ausführung möglicherweise das Ende der retrieveUserMailConfigurationFunktion erreicht. An diesem Punkt ist es ein Fehler, keine returnAnweisung zu haben . Das dort RuntimeExceptiongeworfene soll diese Sorge des Compilers lindern, aber es ist ein ziemlich klobiger Weg, es zu tun. Eine andere Möglichkeit, den function must return a valueFehler zu verhindern , besteht darin, eine return null; //to keep the compiler happyErklärung hinzuzufügen , die meiner Meinung nach jedoch ebenso umständlich ist.

Also persönlich würde ich das ersetzen:

rethrow(e);

mit diesem:

report(e); 
throw e;

oder, noch besser, (wie Coredump angedeutet hat) mit diesem:

throw reportAndTransform(e);

Auf diese Weise würde der Steuerungsfluss für den Compiler offensichtlich, sodass Ihr Final throw new RuntimeException("cannot reach here");nicht nur redundant, sondern auch nicht kompilierbar wird, da es vom Compiler als nicht erreichbarer Code gekennzeichnet wird.

Das ist der eleganteste und eigentlich auch einfachste Weg, aus dieser hässlichen Situation herauszukommen.

Das throwwurde wahrscheinlich hinzugefügt, um den Fehler "Methode muss einen Wert zurückgeben" zu umgehen, der andernfalls auftreten würde - der Java-Datenflussanalysator ist intelligent genug, um zu verstehen, dass returnnach einem throw, aber nicht nach Ihrer benutzerdefinierten rethrow()Methode, keine @NoReturnAnnotation erforderlich ist dass Sie dies beheben können.

Das Erstellen einer neuen Ausnahme in nicht erreichbarem Code erscheint jedoch überflüssig. Ich würde einfach schreiben return null, wissend, dass es in der Tat nie passiert.

Das

throw new RuntimeException("cannot reach here");

Die Anweisung macht einem PERSONEN , der den Code liest, klar, was gerade passiert. Es ist also viel besser, als beispielsweise null zurückzugeben. Dies erleichtert auch das Debuggen, wenn der Code auf unerwartete Weise geändert wird.

Scheint aber rethrow(e)einfach falsch! In Ihrem Fall halte ich das Refactoring des Codes für eine bessere Option. In den anderen Antworten (ich mag die besten von coredump) erfahren Sie, wie Sie Ihren Code sortieren können.

Ich weiß nicht, ob es eine Konvention gibt.

Jedenfalls wäre ein anderer Trick, so zu tun:

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

Dies zulassen:

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

Und keine künstliche RuntimeExceptionwird mehr benötigt. Obwohl es rethroweigentlich nie einen Wert zurückgibt, ist es jetzt gut genug für den Compiler.

Es gibt theoretisch einen Wert zurück (Methodensignatur) und ist dann davon ausgenommen, da es stattdessen eine Ausnahme auslöst.

Ja, es mag seltsam aussehen, aber RuntimeExceptionandererseits - ein Phantom zu werfen oder Nullen zurückzugeben, die in dieser Welt niemals zu sehen sind - ist auch nicht gerade etwas Schönes.

Wenn Sie die Lesbarkeit anstreben, können Sie Folgendes umbenennen rethrowund haben:

} catch (Exception e) {
     return nothingJustRethrow(e);
}

Wenn Sie den tryBlock komplett entfernen, brauchen Sie das rethrowoder das nicht throw. Dieser Code macht genau das Gleiche wie das Original:

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

Lassen Sie sich nicht von der Tatsache täuschen, dass es von erfahreneren Entwicklern stammt. Das ist Code Rot, und das passiert die ganze Zeit. Repariere es einfach.

EDIT: Ich habe falsch gelesen, rethrow(e)als würde ich die Ausnahme einfach neu auslösen e. Wenn diese rethrowMethode tatsächlich etwas anderes ausführt als die Ausnahme erneut auszulösen, ändert das Entfernen der Ausnahme die Semantik dieser Methode.