risposta-alla-domanda-sullo-sviluppo-web-bd.com

Restituire un'eccezione è un anti-modello?

Ho due metodi semplici:

public void proceedWhenError() {
   Throwable exception = serviceUp();

   if (exception == null) {
      // do stuff
   } else {
      logger.debug("Exception happened, but it's alright.", exception)
      // do stuff
   }
}

public void doNotProceedWhenError() {
   Throwable exception = serviceUp();

   if (exception == null) {
      // do stuff
   } else {
      // do stuff
      throw new IllegalStateException("Oh, we cannot proceed. The service is not up.", exception)
   }
}

Il terzo metodo è un metodo di supporto privato:

private Throwable serviceUp() {
    try {
        service.connect();
        return null;
    catch(Exception e) {
       return e;
    }
}

Abbiamo avuto una chiacchierata con un mio collega sul modello usato qui:

restituire un oggetto Exception (o Throwable) dal metodo serviceUp().

Il primo parere:

È un anti-pattern per usare Exceptions per controllare il flusso di lavoro e dovremmo restituire boolean solo da serviceUp() e mai l'oggetto Exception stesso. L'argomento è che usare Eccezioni per controllare il flusso di lavoro è un anti-modello.

Il secondo parere:

Va bene, dato che dobbiamo occuparci dell'oggetto in seguito nel due primi metodi in modo diverso e se restituire l'oggetto Exception o booleano non cambia affatto il flusso di lavoro

Pensi che 1) o 2) sia corretto e soprattutto, perché? Nota che la domanda è SOLO sul metodo serviceUp() e il suo tipo di ritorno - boolean vs Exception oggetto.

Nota: non sto chiedendo se utilizzare oggetti Throwable o Exception.

19
Martin Linha

È un anti-pattern per usare le eccezioni per dirigere il flusso solo quando l'eccezione viene lanciata in una situazione non eccezionale*. Ad esempio, terminare un ciclo generando un'eccezione quando raggiungi la fine di una raccolta è un anti-pattern.

Il controllo del flusso con le eccezioni reali, d'altra parte, è una buona applicazione di eccezioni. Se il tuo metodo incontra una situazione eccezionale che non può gestire, dovrebbe generare un'eccezione, quindi reindirizzare il flusso nel chiamante al blocco del gestore delle eccezioni.

Restituire un oggetto "Exception" nudo da un metodo, anziché lanciarlo, è certamente contro-intuitivo. Se è necessario comunicare i risultati di un'operazione al chiamante, un approccio migliore consiste nell'utilizzare un oggetto di stato che include tutte le informazioni pertinenti, inclusa l'eccezione:

public class CallStatus {
    private final Exception serviceException;
    private final boolean isSuccess;
    public static final CallStatus SUCCESS = new CallStatus(null, true);
    private CallStatus(Exception e, boolean s) {
        serviceException = e;
        isSuccess = s;
    }
    public boolean isSuccess() { return isSuccess; }
    public Exception getServiceException() { return serviceException; }
    public static CallStatus error(Exception e) {
        return new CallStatus(e, false);
    }
}

Ora il chiamante riceverà CallStatus da serviceUp:

CallStatus status = serviceUp();
if (status.isSuccess()) {
    ... // Do something
} else {
    ... // Do something else
    Exception ex = status.getException();
}

Si noti che il costruttore è privato, quindi serviceUp potrebbe restituire CallStatus.SUCCESS o chiamare CallStatus.error(myException).

* Ciò che è eccezionale e ciò che non è eccezionale dipende molto dal contesto. Ad esempio, i dati non numerici causano un'eccezione in Scanner 'nextInt, perché considera tali dati non validi. Tuttavia, lo stesso esatto dato non causa un'eccezione nel metodo hasNextInt, perché è perfettamente valido.

30
dasblinkenlight

La seconda opinione ("va tutto bene") non regge. Il codice non va bene perché restituire le eccezioni invece di lanciarle non è veramente idiomatico.

Inoltre non compro il primo parere ("usare le eccezioni per controllare il flusso di lavoro è anti-pattern"). service.connect() sta lanciando un'eccezione e devi reagire a questa eccezione - quindi questo è controllo di flusso efficace. Restituire un boolean o qualche altro oggetto di stato ed elaborarlo invece di gestire un'eccezione e pensare che non sia il flusso di controllo basato su un'eccezione è ingenuo. Un altro svantaggio è che se decidi di rilanciare un'eccezione (racchiusa in IllegalArgumentException o qualsiasi altra cosa), non avrai più l'eccezione originale. E questo è estremamente brutto quando si tenta di analizzare cosa è realmente accaduto.

Quindi farei il classico:

  • Lancia l'eccezione in serviceUp.
  • Nei metodi che invocano serviceUp:
    • try-catch, registra il debug e ingoia l'eccezione se vuoi procedere con l'eccezione.
    • try-catch e ripropone l'eccezione avvolta in un'altra eccezione che fornisce maggiori informazioni su cosa è successo. In alternativa lascia che l'eccezione originale si propaghi via throws se non puoi aggiungere nulla di sostanziale.

È molto importante non perdere l'eccezione originale.

17
lexicore

Entrambi hanno torto

Va bene, dato che abbiamo bisogno di trattare l'oggetto in seguito nei due primi metodi in modo diverso e se restituire l'oggetto Exception o booleano non cambia affatto il flusso di lavoro

Non va bene. Il concetto di eccezioni significa che vengono gettati in, benissimo, casi eccezionali. Sono pensati per essere catturati nel luogo in cui saranno gestiti (o almeno re-gettato dopo una pulizia/registrazione/ecc.). Non sono pensati per essere consegnati in questo modo (cioè nel codice "dominio").

Le persone saranno confuse. I veri bug possono facilmente insinuarsi - per esempio, cosa succede se c'è qualche Exception da qualche altra fonte rispetto alla rete qui; quello che non avevate previsto, è davvero pessimo (come un'eccezione out-of-bounds creata da un errore di programmazione)?

E i nuovi programmatori saranno confusi all'infinito e/o copieranno l'anti-modello in posti in cui non appartiene.

Ad esempio, un collega ha recentemente implementato un'interfaccia piuttosto complicata (come nell'interfaccia machine-to-machine, non Java interface), e ha eseguito una gestione delle eccezioni simile; convertire le eccezioni in variazioni silenziosamente ignorate (modulo alcuni messaggi di log). Inutile dire che qualsiasi eccezione che non si aspettava davvero di aver rotto l'intero casino nel peggior modo immaginabile; il contrario di fail fast .

È un anti-pattern per utilizzare le eccezioni per controllare il flusso di lavoro e dovremmo restituire booleano solo da serviceUp () e mai l'oggetto Exception stesso. L'argomento è che l'uso di Eccezioni per controllare il flusso di lavoro è un anti-pattern.

Le eccezioni controllano sicuramente il flusso di lavoro nell'aspetto che spesso lo abortiscono o reindirizzano a un messaggio di errore visualizzato all'utente. È assolutamente possibile avere una parte del codice "disabilitata" a causa di un'eccezione; Ad esempio, la gestione delle eccezioni è sicuramente consentita da qualche altra parte rispetto al solo livello di controllo superiore.

Ma restituire le eccezioni è in effetti un anti-modello; nessuno si aspetta che, è strano, porta a errori spuri (è facile ignorare il valore di ritorno) ecc. ecc.

Quindi, nel caso del tuo serviceUp(), rendilo void - funziona il 99% delle volte o genera un'eccezione; o rendi vero boolean in quanto accetti pienamente che fallirà da qualche parte. Se hai bisogno di consegnare il messaggio di errore, fallo come un String, o salvalo da qualche parte fuori strada, o qualcosa del genere, ma non usarlo come valore di ritorno, specialmente non se intendi throw di nuovo in seguito.

Soluzione semplice e standard

Questa soluzione è più corta (meno linee, meno variabili, meno if), più semplice, bog-standard e fa esattamente quello che volevi. Facile da mantenere, facile da capire. 

public void proceedWhenError() {
   try {
      serviceUp();
      // do stuff (only when no exception)
   }
   catch (Exception exception) {
      logger.debug("Exception happened, but it's alright.", exception)
      // do stuff (only when exception)
   }
}

public void doNotProceedWhenError() {
   try {
      serviceUp();
      // do stuff (only when no exception)
   }
   catch (Exception exception) {
      // do stuff (only when exception)
      throw new IllegalStateException("Oh, we cannot proceed. The service is not up.", exception)
   }
}

private void serviceUp() {
    service.connect();
}
7
AnoE

Vorrei restituire un ServiceState che può essere, ad esempio, RUNNING, WAITING, CLOSED. Il metodo sarebbe chiamato getServiceState.

enum ServiceState { RUNNING, WAITING, CLOSED; }

Non ho mai visto i metodi che restituiscono un'eccezione a seguito dell'esecuzione. Per me, quando un metodo restituisce il valore, significa che il metodo ha terminato il suo lavoro senza problemi . Non voglio recuperare il risultato e analizzarlo per contenere eventuali errori. Il risultato stesso significa che non si sono verificati errori - tutto è andato come previsto.

D'altra parte, quando il metodo genera un'eccezione, devo analizzare un special object per capire cosa è andato storto. Non analizzo il risultato, perché non c'è risultato .

Un esempio:

public void proceedWhenError() {
   final ServiceState state = getServiceState();

   if (state != ServiceState.RUNNING) {
      logger.debug("The service is not running, but it's alright.");
   }
   // do stuff
}

public void doNotProceedWhenError() {
   final ServiceState state = getServiceState();

   if (state != ServiceState.RUNNING) {
      throw new IllegalStateException("The service is not running...");
   }
   // do stuff
}

private ServiceState getServiceState() {
    try {
        service.connect();
        return ServiceState.RUNNING;
    catch(Exception e) {
        // determine the state by parsing the exception
        // and return it
        return getStateFromException(e);
    }
}

Se le eccezioni lanciate dal servizio sono importanti e/o elaborate in un altro luogo, insieme allo stato potrebbero essere salvate in un oggetto ServiceResponse:

class ServiceResponse {

    private final ServiceState state;
    private final Exception exception;

    public ServiceResponse(ServiceState state, Exception exception) {
        this.state = state;
        this.exception = exception;
    }

    public static ServiceResponse of(ServiceState state) {
        return new ServiceResponse(state, null);
    }

    public static ServiceResponse of(Exception exception) {
        return new ServiceResponse(null, exception);
    }

    public ServiceState getState() {
        return state;
    }

    public Exception getException() {
        return exception;
    }

}

Ora, con ServiceResponse, questi metodi potrebbero avere il seguente aspetto:

public void proceedWhenError() {
   final ServiceResponse response = getServiceResponse();

   final ServiceState state = response.getState();
   final Exception exception = response.getException();

   if (state != ServiceState.RUNNING) {
      logger.debug("The service is not running, but it's alright.", exception);
   }
   // do stuff
}

public void doNotProceedWhenError() {
   final ServiceResponse response = getServiceResponse();

   final ServiceState state = response.getState();
   final Exception exception = response.getException();

   if (state != ServiceState.RUNNING) {
      throw new IllegalStateException("The service is not running...", exception);
   }
   // do stuff
}

private ServiceResponse getServiceResponse() {
    try {
        service.connect();
        return ServiceResponse.of(ServiceState.RUNNING);
    catch(Exception e) {
        // or -> return ServiceResponse.of(e);
        return new ServiceResponse(getStateFromException(e), e);
    }
}
3
Andrew Tobilko

L'evidente dissonanza cognitiva è l'anti-modello qui. Un lettore ti vedrà usando le eccezioni per il controllo del flusso e uno sviluppatore proverà immediatamente a ricodificarlo in modo che non lo faccia.

Il mio istinto suggerisce un approccio come:

// Use an action name instead of a question here because it IS an action.
private void bringServiceUp() throws Exception {

}

// Encapsulate both a success and a failure into the result.
class Result {
    final boolean success;
    final Exception failure;

    private Result(boolean success, Exception failure) {
        this.success = success;
        this.failure = failure;
    }

    Result(boolean success) {
        this(success, null);
    }

    Result(Exception exception) {
        this(false, exception);
    }

    public boolean wasSuccessful() {
        return success;
    }

    public Exception getFailure() {
        return failure;
    }
}

// No more cognitive dissonance.
private Result tryToBringServiceUp() {
    try {
        bringServiceUp();
    } catch (Exception e) {
        return new Result(e);
    }
    return new Result(true);
}

// Now these two are fine.
public void proceedWhenError() {
    Result result = tryToBringServiceUp();
    if (result.wasSuccessful()) {
        // do stuff
    } else {
        logger.debug("Exception happened, but it's alright.", result.getFailure());
        // do stuff
    }
}

public void doNotProceedWhenError() throws IllegalStateException {
    Result result = tryToBringServiceUp();
    if (result.wasSuccessful()) {
        // do stuff
    } else {
        // do stuff
        throw new IllegalStateException("Oh, we cannot proceed. The service is not up.", result.getFailure());
    }
}
1
OldCurmudgeon

Se i chiamanti di un metodo si aspetteranno che un'operazione possa avere esito positivo o negativo e siano pronti a gestire entrambi i casi, il metodo dovrebbe indicare, in genere tramite valore di ritorno, se l'operazione ha avuto esito negativo. Se i chiamanti non sono preparati a gestire l'errore, il metodo in cui si è verificato l'errore deve generare l'eccezione anziché richiedere ai chiamanti di aggiungere codice per farlo.

L'unica ruga è che alcuni metodi avranno alcuni chiamanti che sono pronti a gestire con grazia i guasti e altri che non lo sono. Il mio approccio preferito sarebbe che tali metodi accettassero un callback opzionale che viene richiamato in caso di errore; se non viene fornita alcuna richiamata, il comportamento predefinito dovrebbe essere quello di generare un'eccezione. Un simile approccio salverebbe il costo della costruzione di un'eccezione nei casi in cui un chiamante è pronto a gestire un errore, riducendo al minimo l'onere sui chiamanti che non lo sono. La più grande difficoltà con un tale progetto è decidere quali parametri dovrebbe prendere una tale callback, poiché modificare tali parametri in un secondo momento può essere difficile.

1
supercat

restituire un'eccezione è in effetti un pattern anti perché le eccezioni dovrebbero essere riservate per errori nell'esecuzione, non per descrivere le condizioni del servizio.

immagina se c'è un bug nel codice di serviceUp() che causa il lancio di NullPointerException. Ora immagina che il bug sia nel servizio e lo stesso NullPointerException viene generato da connect()

Vedi il mio punto?

Un'altra ragione sta cambiando i requisiti. 

Attualmente, il servizio ha due condizioni: su o giù.
Attualmente.

Tommorow, avrai tre condizioni per il servizio: su, giù. o funzionante con avvertimenti. Il giorno dopo, vorrai anche il metodo per restituire dettagli sul servizio in json .....

1
Sharon Ben Asher

Come menzionato nei commenti precedenti "L'eccezione è un evento" l'oggetto di eccezione che otteniamo è solo un dettaglio dell'evento . Una volta che un'eccezione viene catturata nel blocco "catch" e non re-generata, l'evento è finitoPost che hai solo un oggetto di dettaglio non un'eccezione, sebbene l'oggetto sia di eccezione di classe/gettabile.

Restituire l'oggetto di eccezione potrebbe essere utile a causa dei dettagli in esso contenuti, ma aggiungerà ambiguità/confusione poiché il metodo di chiamata non "gestisce l'eccezione e controlla il flusso in base all'eccezione". Si tratta semplicemente di utilizzare i dettagli di un oggetto restituito per prendere decisioni.

Quindi, a mio parere, un modo più logico e meno confusionario sarà quello di restituire un booleano/enum basato sull'eccezione piuttosto che semplicemente restituire un oggetto di eccezione. 

0
dev. K

Questo è un anti-pattern:

catch(Exception e) {
   return e;
}

L'unica scusa ragionevole per restituire Throwable è di fornire informazioni dettagliate sull'errore a un percorso veloce in cui è previsto un errore, senza pagare il prezzo1 di gestione delle eccezioni. Come bonus, rende facile convertire in un'eccezione generata se il chiamante non sa come gestire questa particolare situazione.

Ma nel tuo scenario, quel costo è già stato pagato. Catturare l'eccezione per conto del chiamante non fa alcun favore.


1Pedanticamente, il costo diretto di catturare un'eccezione (trovare un gestore corrispondente, impilare lo svolgimento) è piuttosto basso o comunque necessario. La maggior parte del costo di try/catch rispetto a un valore di ritorno è in azioni accidentali, come la creazione di una traccia di stack. Quindi, se gli oggetti eccezionalmente non sviluppati fanno una buona archiviazione per dati di errore efficienti dipende dal fatto che questo lavoro occasionale sia fatto al momento della costruzione o al momento del lancio. Pertanto, se la restituzione di un oggetto di eccezione è ragionevole, può variare a seconda delle diverse piattaforme gestite.

0
Ben Voigt

Non puoi dire a 2) che l'opinione è falsa, perché il flusso di lavoro verrà eseguito come vuoi che venga eseguito. Da un punto di vista logico verrà eseguito come desiderato, quindi è corretto.

Ma è un modo davvero strano e non consigliato per farlo. Innanzitutto perché l'eccezione non è progettata per farlo (il che significa che stai facendo un anti-pattern). È un oggetto specifico progettato per essere lanciato e catturato. Quindi è strano, piuttosto usato come progettato, scegliere di restituirlo e piuttosto prenderlo usa un se sopra. Inoltre (forse è trascurabile) affronterai un problema di prestazioni piuttosto che usare un semplice booleano come flag, istanzia un intero oggetto.

Infine, è anche sconsigliato che la funzione causa restituisca qualcosa se l'obiettivo della funzione è ottenere qualcosa (che non è il tuo caso). Dovresti riprogettarlo per essere una funzione che avvia un servizio, quindi non restituisce nulla perché non verrà chiamato per ottenere qualcosa, e genererà un'eccezione se si verifica un errore. E se vuoi sapere se il tuo servizio funziona crea una funzione progettata per darti queste informazioni come public boolean isServiceStarted().

0
vincrichaud