Il lancio di nuove RuntimeException nel codice irraggiungibile è un cattivo stile?

Innanzitutto, grazie per aver verificato la tua domanda e mostrandoci cosa fa rethrow . Quindi, in effetti, ciò che fa è convertire le eccezioni con le proprietà in classi di eccezioni più dettagliate. Maggiori informazioni su questo più tardi.

Poiché non ho risposto in modo originale alla domanda principale in origine, eccola: sì, è in generale uno stile non valido per generare eccezioni di runtime in codice non raggiungibile; faresti meglio ad usare asserzioni, o meglio ancora, ad evitare il problema. Come già sottolineato, il compilatore qui non può essere sicuro che il codice non esce mai dal blocco try/catch . Puoi rifattorizzare il tuo codice sfruttando il fatto che ...

Gli errori sono valori

^{(Non sorprendentemente, è ben noto in go )}

Usiamo un esempio più semplice, quello che ho usato prima della modifica: quindi immagina di loggare qualcosa e creare un'eccezione wrapper come in la risposta di Konrad . Chiamiamolo logAndWrap .

Invece di lanciare l'eccezione come effetto collaterale di logAndWrap , puoi lasciare che faccia il suo lavoro come effetto collaterale e fargli restituire un'eccezione (almeno, quella data in input). Non è necessario utilizzare i generici, solo le funzioni di base:

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

Quindi, throw esplicitamente, e il tuo compilatore è soddisfatto:

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

Che cosa succede se ti dimentichi di throw ?

Come spiegato in Il commento di Joe23 , un programmazione difensiva modo per garantire che l'eccezione sia sempre generata consisterebbe nel fare esplicitamente un throw new CustomWrapperException(exception) alla fine di logAndWrap , come è fatto di Guava .Throwables . In questo modo, sai che l'eccezione verrà lanciata e il tuo tipo di analizzatore è felice. Tuttavia, le eccezioni personalizzate devono essere eccezioni non controllate, il che non è sempre possibile. Inoltre, considero il rischio che uno sviluppatore manchi di scrivere throw sia molto basso: lo sviluppatore deve dimenticarlo e il metodo circostante non dovrebbe restituire nulla, altrimenti il compilatore potrebbe rilevare un mancante ritorno. Questo è un modo interessante per combattere il sistema di tipi, e funziona, però.

rigenerare

Anche l'attuale rethrow può essere scritto come una funzione, ma ho problemi con la sua attuale implementazione:

• Ci sono molti cast inutili come Sono infatti necessari i cast (vedi commenti):

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

• Quando si lanciano / ritornano nuove eccezioni, la vecchia viene scartata; nel codice seguente, un MailLoginNotAvailableException non mi consente di sapere quale accesso non è disponibile, il che è inopportuno; inoltre, gli stacktraces saranno incompleti:

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

• Perché il codice di origine non genera quelle eccezioni specializzate in primo luogo? Sospetto che rethrow sia usato come livello di compatibilità tra un sottosistema (mailing) e una logica di busineess (forse l'intento è quello di nascondere i dettagli di implementazione come eccezioni generate sostituendole con eccezioni personalizzate). Anche se sono d'accordo che sarebbe meglio avere il codice catch-free, come suggerito nella risposta di Pete Becker , I non credo che avrai l'opportunità di rimuovere il codice catch e rethrow qui senza importanti refactoring.

Questa funzione rethrow(e); viola il principio che dice che in circostanze normali, una funzione ritornerà, mentre in circostanze eccezionali, una funzione genererà un'eccezione. Questa funzione viola questo principio lanciando un'eccezione in circostanze normali. Questa è la fonte di tutta la confusione.

Il compilatore presume che questa funzione ritorni in circostanze normali, quindi per quanto il compilatore può dire, l'esecuzione può raggiungere la fine della funzione retrieveUserMailConfiguration , a quel punto è un errore non avere un return dichiarazione. Il RuntimeException generato dovrebbe alleviare questa preoccupazione del compilatore, ma è un modo piuttosto goffo di farlo. Un altro modo per evitare l'errore function must return a value è di aggiungere un'istruzione return null; //to keep the compiler happy , ma a mio parere è ugualmente goffo.

Quindi, personalmente, vorrei sostituire questo:

rethrow(e);

con questo:

report(e); 
throw e;

o, meglio ancora, (come suggerito da coredump ,) con questo:

throw reportAndTransform(e);

Quindi, il flusso di controllo sarebbe reso ovvio al compilatore, quindi il tuo throw new RuntimeException("cannot reach here"); finale diventerebbe non solo ridondante, ma in realtà nemmeno compilabile, poiché sarebbe contrassegnato dal compilatore come codice irraggiungibile.

Questo è il modo più elegante ed anche più semplice per uscire da questa brutta situazione.

Il throw è stato probabilmente aggiunto per aggirare l'errore "il metodo deve restituire un valore" che altrimenti si verificherebbe - l'analizzatore del flusso di dati Java è abbastanza intelligente da capire che nessuna return è necessaria dopo un throw , ma non dopo il tuo metodo personalizzato rethrow() , e non c'è nessuna annotazione @NoReturn che potresti utilizzare per risolvere il problema.

Tuttavia, la creazione di una nuova eccezione in codice non raggiungibile sembra superflua. Scriverò semplicemente return null , sapendo che, in effetti, non succede mai.

Il

throw new RuntimeException("cannot reach here");

la dichiarazione rende chiaro a una PERSON lettura del codice cosa sta succedendo, quindi è molto meglio quindi restituire null, ad esempio. Semplifica anche il debug se il codice viene modificato in modo inaspettato.

Tuttavia rethrow(e) sembra solo sbagliato! Quindi, nel tuo caso , ritengo che il refactoring del codice sia un'opzione migliore. Vedi le altre risposte (mi piace il meglio di coredump) per i modi per ordinare il tuo codice.

Non so se c'è una convenzione.

In ogni caso, un altro trucco sarebbe fare così:

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

Consentire questo:

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

E non è più necessario alcun RuntimeException artificiale. Anche se rethrow non restituisce mai alcun valore, ora è abbastanza buono per il compilatore.

Restituisce un valore in teoria (firma del metodo), quindi è esentato dal farlo mentre invece genera un'eccezione.

Sì, potrebbe sembrare strano, ma ancora una volta - lanciando un fantomatico RuntimeException , o restituendo valori null che non saranno mai visti in questo mondo - neanche questo è esattamente un aspetto di bellezza.

Per migliorare la leggibilità, puoi rinominare rethrow e avere qualcosa del tipo:

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

Se rimuovi completamente il blocco try , non hai bisogno di rethrow o throw . Questo codice fa esattamente la stessa cosa dell'originale:

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

Non lasciare ingannare il fatto che provenga da sviluppatori più esperti. Questo è il codice rot, e succede sempre. Basta sistemarlo.

EDIT: I mis-read rethrow(e) come semplicemente rilanciare l'eccezione e . Se quel metodo rethrow fa effettivamente qualcosa di diverso da rilanciare l'eccezione, allora liberarsene cambia la semantica di questo metodo.