Perché un metodo non dovrebbe generare più tipi di eccezioni controllate?

44

Usiamo SonarQube per analizzare il nostro codice Java e ha questa regola (impostata su critica):

Public methods should throw at most one checked exception

Using checked exceptions forces method callers to deal with errors, either by propagating them or by handling them. This makes those exceptions fully part of the API of the method.

To keep the complexity for callers reasonable, methods should not throw more than one kind of checked exception."

Un altro bit in Sonar ha questo :

Public methods should throw at most one checked exception

Using checked exceptions forces method callers to deal with errors, either by propagating them or by handling them. This makes those exceptions fully part of the API of the method.

To keep the complexity for callers reasonable, methods should not throw more than one kind of checked exception.

The following code:

public void delete() throws IOException, SQLException {      // Non-Compliant
  /* ... */
}

should be refactored into:

public void delete() throws SomeApplicationLevelException {  // Compliant
    /* ... */
}

Overriding methods are not checked by this rule and are allowed to throw several checked exceptions.

Non ho mai letto questa regola / raccomandazione nelle mie letture sulla gestione delle eccezioni e ho cercato di trovare alcuni standard, discussioni ecc. sull'argomento. L'unica cosa che ho trovato è questa da CodeRach: Quante eccezioni dovrebbe essere un metodo per più?

Questo è uno standard ben accettato?

    
posta sdoca 26.11.2014 - 21:56
fonte

4 risposte

31

Consideriamo la situazione in cui hai il codice fornito di:

public void delete() throws IOException, SQLException {      // Non-Compliant
  /* ... */
}

Il pericolo qui è che il codice che scrivi per chiamare delete() sarà simile a:

try {
  foo.delete()
} catch (Exception e) {
  /* ... */
}

Anche questo è male. E verrà catturato con un'altra regola che i flag catturano la classe Exception di base.

La chiave è non scrivere codice che ti fa venir voglia di scrivere codice cattivo altrove.

La regola che stai incontrando è piuttosto comune. Checkstyle ha le sue regole di progettazione:

ThrowsCount

Restricts throws statements to a specified count (1 by default).

Rationale: Exceptions form part of a method's interface. Declaring a method to throw too many differently rooted exceptions makes exception handling onerous and leads to poor programming practices such as writing code like catch(Exception ex). This check forces developers to put exceptions into a hierarchy such that in the simplest case, only one type of exception need be checked for by a caller but any subclasses can be caught specifically if necessary.

Questo descrive esattamente il problema e qual è il problema e perché non dovresti farlo. È uno standard ben accettato che molti strumenti di analisi statica identificheranno e contrassegneranno.

E mentre potresti farlo secondo il design del linguaggio, e potresti essere volte in cui è la cosa giusta da fare, è qualcosa che dovresti vedere e vai subito "ehm, perché sto facendo questo?" Può essere accettabile per il codice interno in cui tutti sono disciplinati abbastanza da non catch (Exception e) {} , ma il più delle volte ho visto le persone tagliare le curve soprattutto in situazioni interne.

Non fare in modo che le persone che usano la tua classe vogliano scrivere codice errato.

Devo sottolineare che l'importanza di ciò è ridotta con Java SE 7 e successivi perché una singola istruzione catch può catturare più eccezioni (Catching Multiple Exception Types e Rethrowing Exceptions con Migliorato il controllo del tipo da Oracle).

Con Java 6 e precedenti, avresti un codice simile a:

public void delete() throws IOException, SQLException {
  /* ... */
}

e

try {
  foo.delete()
} catch (IOException ex) {
     logger.log(ex);
     throw ex;
} catch (SQLException ex) {
     logger.log(ex);
     throw ex;
}

o

try {
    foo.delete()
} catch (Exception ex) {
    logger.log(ex);
    throw ex;
}

Nessuna di queste opzioni con Java 6 è l'ideale. Il primo approccio viola DRY . Blocchi multipli che fanno la stessa cosa, ancora e ancora - una volta per ogni eccezione. Vuoi registrare l'eccezione e ricrearla? Ok. Le stesse linee di codice per ogni eccezione.

La seconda opzione è peggiore per diversi motivi. Primo, significa che stai prendendo tutte le eccezioni. Puntatore nullo viene catturato lì (e non dovrebbe). Inoltre, stai rilanciando un Exception , il che significa che la firma del metodo sarebbe deleteSomething() throws Exception , il che rende ancora più complicato lo stack poiché le persone che usano il tuo codice ora sono forzate a catch(Exception e) .

Con Java 7, questo non è come importante perché puoi invece fare:

catch (IOException|SQLException ex) {
    logger.log(ex);
    throw ex;
}

Inoltre, il tipo controlla se uno fa cattura i tipi di eccezioni lanciate:

public void rethrowException(String exceptionName)
throws IOException, SQLException {
    try {
        foo.delete();
    } catch (Exception e) {
        throw e;
    }
}

Il type checker riconoscerà che e potrebbe solo essere di tipi IOException o SQLException . Non sono ancora eccessivamente entusiasta dell'uso di questo stile, ma non sta causando un codice errato come lo era in Java 6 (dove ti costringerebbe ad avere la firma del metodo come la superclasse che le eccezioni estendono).

Nonostante tutti questi cambiamenti, molti strumenti di analisi statica (Sonar, PMD, Checkstyle) stanno ancora applicando le guide in stile Java 6. Non è una brutta cosa Tendo ad essere d'accordo con un avvertimento per far sì che ancora venga applicato, ma potresti cambiare la priorità su di loro in maggiore o in minore in base al modo in cui il team li assegna per priorità.

Se le eccezioni devono essere selezionate o deselezionate ... si tratta di g r e a t dibattito che si può facilmente trovare innumerevoli post sul blog che occupano entrambi i lati dell'argomento. Tuttavia, se stai lavorando con eccezioni controllate, probabilmente dovresti evitare di lanciare più tipi, almeno in Java 6.

    
risposta data 27.11.2014 - 01:59
fonte
21

Il motivo per cui preferisci, in teoria, lanciare un solo tipo di eccezione è perché altrimenti è probabile che violi la Single Responsibility e Inversione di dipendenza . Usiamo un esempio per dimostrare.

Diciamo che abbiamo un metodo che recupera i dati dalla persistenza e che la persistenza è un insieme di file. Dato che abbiamo a che fare con i file, possiamo avere FileNotFoundException :

public String getData(int id) throws FileNotFoundException

Ora abbiamo un cambiamento nei requisiti e i nostri dati provengono da un database. Invece di FileNotFoundException (dato che non abbiamo a che fare con i file), gettiamo un SQLException :

public String getData(int id) throws SQLException

Ora dovremmo esaminare tutto il codice che utilizza il nostro metodo e modificare l'eccezione da controllare, altrimenti il codice non verrà compilato. Se il nostro metodo viene chiamato in lungo e in largo, può essere molto cambiare / far cambiare gli altri. Ci vuole un sacco di tempo e le persone non saranno felici.

L'inversione delle dipendenze dice che non dovremmo davvero lanciare nessuna di queste eccezioni perché espongono i dettagli di implementazione interna che stiamo lavorando per incapsulare. Il codice di chiamata deve sapere quale tipo di persistenza stiamo usando, quando in realtà dovrebbe essere solo preoccupato se il record può essere recuperato. Dovremmo invece lanciare un'eccezione che trasmetta l'errore allo stesso livello di astrazione che stiamo esponendo tramite la nostra API:

public String getData(int id) throws InvalidRecordException

Ora, se cambiamo l'implementazione interna, possiamo semplicemente avvolgere quell'eccezione in InvalidRecordException e passarla (o non avvolgerla, e lanciare solo una nuova InvalidRecordException ). Il codice esterno non sa o si preoccupa di quale tipo di persistenza viene utilizzato. È tutto incapsulato.

Per quanto riguarda la responsabilità unica, dobbiamo pensare al codice che genera eccezioni multiple, non correlate. Diciamo che abbiamo il seguente metodo:

public Record parseFile(String filename) throws IOException, ParseException

Che cosa possiamo dire di questo metodo? Possiamo dire solo dalla firma che apre un file e lo analizza. Quando vediamo una congiunzione, come "e" o "o" nella descrizione di un metodo, sappiamo che sta facendo più di una cosa; ha più di una responsabilità . I metodi con più di una responsabilità sono difficili da gestire in quanto possono cambiare se cambiano le responsabilità. Invece, dovremmo rompere i metodi in modo che abbiano una sola responsabilità:

public String readFile(String filename) throws IOException
public Record parse(String data) throws ParseException

Abbiamo estratto la responsabilità di leggere il file dalla responsabilità di analizzare i dati. Un effetto collaterale di questo è che ora possiamo passare tutti i dati String ai dati di analisi da qualsiasi sorgente: in-memory, file, rete, ecc. Ora possiamo anche testare parse più facilmente perché non abbiamo bisogno di un file su disco per eseguire test contro.

A volte ci sono davvero due (o più) eccezioni che possiamo lanciare da un metodo, ma se ci atteniamo a SRP e DIP, i tempi in cui ci imbattiamo in questa situazione diventano più rari.

    
risposta data 29.11.2014 - 06:56
fonte
3

Ricordo di aver giocherellato un po 'con questo quando ho suonato con Java qualche tempo fa, ma non ero consapevole della distinzione tra selezionata e deselezionata finché non ho letto la tua domanda. Ho trovato questo articolo su Google abbastanza rapidamente, e va in alcune delle apparenti polemiche:

link

Detto questo, uno dei problemi che questo ragazzo stava citando con le eccezioni controllate è che (e personalmente mi sono imbattuto in questo fin dall'inizio con Java) se continui ad aggiungere un gruppo di eccezioni controllate alle clausole throws in le tue dichiarazioni di metodo, non solo devi inserire molto più codice per sostenerlo mentre passi a metodi di livello superiore, ma crea anche un disordine maggiore e rompe la compatibilità quando provi a introdurre più tipi di eccezioni per abbassarli -velocità Se aggiungi un tipo di eccezione verificata a un metodo di livello inferiore, devi eseguire nuovamente il codice e regolare anche diverse altre dichiarazioni di metodo.

Un punto di mitigazione menzionato nell'articolo - e all'autore non è piaciuto personalmente - è stato quello di creare un'eccezione di classe base, limitare le tue clausole throws per usarlo solo, e quindi solo aumentare sottoclassi di esso internamente. In questo modo puoi creare nuovi tipi di eccezioni controllati senza dover eseguire di nuovo il codice.

L'autore di questo articolo potrebbe non aver apprezzato molto questo, ma ha perfettamente senso nella mia esperienza personale (specialmente se riesci a vedere quali sono comunque tutte le sottoclassi), e scommetto che è per questo che il consiglio che stai facendo dato è di limitare tutto a un tipo di eccezione verificata ciascuno. Inoltre, i consigli che hai menzionato consentono effettivamente tipi di eccezioni controllati multipli in metodi non pubblici, il che ha perfettamente senso se questo è il loro motivo (o anche altro). Se è solo un metodo privato o qualcosa di simile comunque, non avrai eseguito metà della tua base di codice quando cambi una piccola cosa.

Hai chiesto in gran parte se si tratta di uno standard accettato, ma tra le tue ricerche che hai citato, questo articolo ragionevolmente studiato, e solo parlando dall'esperienza di programmazione personale, non sembra certo che risaltasse in alcun modo.

    
risposta data 26.11.2014 - 22:40
fonte
-1

Lanciare più eccezioni controllate ha senso quando ci sono molte cose ragionevoli da fare.

Per esempio, diciamo che hai un metodo

public void doSomething(Credentials cred, Work work) 
    throws CredentialsRequiredException, TryAgainLaterException{...}

questo viola la regola delle eccezioni pne, ma ha senso.

Purtroppo, quello che succede di solito sono metodi come

void doSomething() 
    throws IOException, JAXBException,SQLException,MyException {...}

Qui ci sono poche possibilità che il chiamante faccia qualcosa di specifico in base al tipo di eccezione. Quindi, se vogliamo spingerlo a realizzare che questi metodi POSSONO, a volte ANDRANNO a sbagliare, lanciare solo SomethingMightGoWrongException è meglio e meglio.

Quindi regola al massimo un'eccezione controllata.

Ma se il progetto utilizza la progettazione in cui sono presenti più eccezioni verificate significative, questa regola non dovrebbe essere applicata.

Sidenote: Qualcosa in realtà può andare storto quasi ovunque, quindi si può pensare di usare? estende RuntimeException, ma c'è una differenza tra "facciamo tutti errori" e "questo parla con il sistema esterno e a volte sarà giù, gestirlo".

    
risposta data 29.11.2014 - 15:06
fonte

Leggi altre domande sui tag