Devo registrare gli errori sui costruttori di lancio delle eccezioni?

15

Stavo costruendo un'applicazione per alcuni mesi e mi sono reso conto di un modello emerso:

logger.error(ERROR_MSG);
throw new Exception(ERROR_MSG);

Oppure, quando si cattura:

try { 
    // ...block that can throw something
} catch (Exception e) {
    logger.error(ERROR_MSG, e);
    throw new MyException(ERROR_MSG, e);
}

Quindi, ogni volta che stavo lanciando o catturando un'eccezione, lo registravo. In effetti, quello era quasi tutto il logging che ho fatto sull'applicazione (oltre a qualcosa per l'inizializzazione dell'applicazione).

Quindi, come programmatore, evito la ripetizione; Così ho deciso di spostare le chiamate del logger nella costruzione delle eccezioni, così, ogni volta che stavo creando un'eccezione, le cose sarebbero state registrate. Potrei anche, certamente, creare un ExceptionHelper che ha gettato l'eccezione per me, ma questo renderebbe il mio codice più difficile da interpretare e, ancora peggio, il compilatore non si comporterebbe bene con esso, non riuscendo a capire che una chiamata a quel membro sarebbe lanciare immediatamente.

Quindi, questo è un anti-pattern? Se sì, perché?

    
posta Bruno Brant 28.12.2015 - 21:18
fonte

8 risposte

17

Non sono sicuro se si qualifica come anti-pattern, ma IMO è una cattiva idea: è un accoppiamento non necessario per intrecciare un'eccezione con la registrazione.

Potresti non voler sempre registrare tutte le istanze di una determinata eccezione (forse si verifica durante la convalida dell'input, la cui registrazione potrebbe essere sia voluminosa che non interessante).

In secondo luogo, potresti decidere di registrare diverse occorrenze di un errore con diversi livelli di registrazione, a quel punto dovresti specificarlo durante la costruzione dell'eccezione, che rappresenta di nuovo la confusione della creazione di eccezioni con il comportamento di registrazione.

Infine, che succede se si verificano eccezioni durante la registrazione di un'altra eccezione? Lo accetterebbe? Diventa caotico ...

Le tue scelte sono fondamentalmente:

  • Catch, log e (re) throw, come hai dato nell'esempio
  • Crea una classe ExceptionHelper per fare entrambe le cose, ma gli aiutanti hanno un odore di codice, e non lo consiglierei neanche.
  • Sposta la gestione delle eccezioni catch-all a un livello superiore
  • Considera AOP per una soluzione più sofisticata ai problemi trasversali come la registrazione e la gestione delle eccezioni (ma lontano più complesso di avere solo queste due linee nei blocchi catch;))
risposta data 28.12.2015 - 23:00
fonte
11

So, as a programmer, I shun repetition [...]

C'è un pericolo qui ogni volta che il concetto di "don't repeat yourself" viene preso un po 'troppo sul serio fino al punto in cui diventa l'odore.

    
risposta data 29.12.2015 - 12:32
fonte
6

Per echo @ Dan1701

Riguarda la separazione delle preoccupazioni - spostare la registrazione nell'eccezione crea un accoppiamento stretto tra l'eccezione e la registrazione e significa anche che hai aggiunto una responsabilità aggiuntiva all'eccezione della registrazione che a sua volta può creare dipendenze per l'eccezione classe di cui non ha bisogno.

Da un punto di vista della manutenzione puoi (direi) che stai nascondendo il fatto che l'eccezione viene registrata dal manutentore (almeno nel contesto di qualcosa come l'esempio), stai anche cambiando il contesto (dalla posizione del gestore delle eccezioni al costruttore dell'eccezione) che è probabilmente non ciò che intendevi.

Infine, si presume che si desideri sempre registrare un'eccezione, esattamente nello stesso modo, nel punto in cui è stata creata / sollevata, il che probabilmente non è il caso. A meno che tu non abbia eccezioni di logging e non-logging che diventerebbero molto spiacevoli abbastanza rapidamente.

Quindi in questo caso penso che "SRP" superi "DRY".

    
risposta data 29.12.2015 - 11:23
fonte
2

Il tuo errore sta registrando un'eccezione in cui non puoi gestirlo, e quindi non puoi sapere se la registrazione è parte della corretta gestione di esso.
Ci sono pochissimi casi in cui è possibile o deve gestire parte dell'errore, che potrebbe includere la registrazione, ma ancora deve segnalare l'errore al chiamante. Gli esempi sono errori di lettura non correggibili e simili, se si esegue la lettura di livello più basso, ma generalmente hanno in comune che le informazioni comunicate al chiamante sono gravemente filtrate, per sicurezza e usabilità.

L'unica cosa che puoi fare nel tuo caso, e per qualche ragione devi fare, è tradurre l'eccezione che l'implementazione getta in una che il chiamante si aspetta, concatenando l'originale per il contesto, e lasciando tutto il resto da solo.

Per riassumere, il tuo codice ha arrogato il diritto e il dovere per la gestione parziale dell'eccezione, violando così l'SRP.
ASCIUTTO non ci entra.

    
risposta data 29.12.2015 - 23:08
fonte
1

Rialzare un'eccezione solo perché hai deciso di registrarla utilizzando un blocco catch (ovvero l'eccezione non è stata affatto modificata) è una cattiva idea.

Uno dei motivi per cui utilizziamo eccezioni, messaggi di eccezioni e la sua gestione è che sappiamo cosa è andato storto e le eccezioni scritte in modo intelligente possono velocizzare il rilevamento del bug con un ampio margine.

Ricorda inoltre che la gestione delle eccezioni costa più risorse di quanto si supponga abbiano if , quindi non dovresti gestirle tutte spesso solo perché ne hai voglia. Ha un impatto sulle prestazioni della tua applicazione.

È comunque un buon approccio usare l'eccezione come mezzo per contrassegnare il livello dell'applicazione in cui è apparso l'errore.

Considera il seguente codice semi-pseudo:

interface ICache<T, U>
{
    T GetValueByKey(U key); // may throw an CacheException
}

class FileCache<T, U> : ICache<T, U>
{
    T GetValueByKey(U key)
    {
        throw new CacheException("Could not retrieve object from FileCache::getvalueByKey. The File could not be opened. Key: " + key);
    }
}

class RedisCache<T, U> : ICache<T, U>
{
    T GetValueByKey(U key)
    {
        throw new CacheException("Could not retrieve object from RedisCache::getvalueByKey. Failed connecting to Redis server. Redis server timed out. Key: " + key);
    }
}

class CacheableInt
{
    ICache<int, int> cache;
    ILogger logger;

    public CacheableInt(ICache<int, int> cache, ILogger logger)
    {
        this.cache = cache;
        this.logger = logger;
    }

    public int GetNumber(int key) // may throw service exception
    {
        int result;

        try {
            result = this.cache.GetValueByKey(key);
        } catch (Exception e) {
            this.logger.Error(e);
            throw new ServiceException("CacheableInt::GetNumber failed, because the cache layer could not respond to request. Key: " + key);
        }

        return result;
    }
}

class CacheableIntService
{
    CacheableInt cacheableInt;
    ILogger logger;

    CacheableInt(CacheableInt cacheableInt, ILogger logger)
    {
        this.cacheableInt = cacheableInt;
        this.logger = logger;
    }

    int GetNumberAndReturnCode(int key)
    {
        int number;

        try {
            number = this.cacheableInt.GetNumber(key);
        } catch (Exception e) {
            this.logger.Error(e);
            return 500; // error code
        }

        return 200; // ok code
    }
}

Supponiamo che qualcuno abbia chiamato GetNumberAndReturnCode e abbia ricevuto il codice 500 , segnalando un errore. Chiamerebbe il supporto, chi aprirà il file di registro e vedrà questo:

ERROR: 12:23:27 - Could not retrieve object from RedisCache::getvalueByKey. Failed connecting to Redis server. Redis server timed out. Key: 28
ERROR: 12:23:27 - CacheableInt::GetNumber failed, because the cache layer could not respond to request. Key: 28

Lo sviluppatore quindi sa immediatamente quale livello del software ha causato l'interruzione del processo e ha un modo semplice per identificare il problema. In questo caso è fondamentale, perché il timeout di Redis non dovrebbe mai accadere.

Forse un altro utente chiamerebbe lo stesso metodo, riceverà anche il codice 500 , ma il registro mostrerebbe quanto segue:

INFO: 11:11:11- Could not retrieve object from RedisCache::getvalueByKey. Value does not exist for the key 28.
INFO: 11:11:11- CacheableInt::GetNumber failed, because the cache layer could not find any data for the key 28.

In questo caso il supporto potrebbe semplicemente rispondere all'utente che la richiesta non è valida perché sta richiedendo un valore per un ID inesistente.

Sommario

Se gestisci le eccezioni, assicurati di gestirle nel modo corretto. Assicurati inoltre che le eccezioni includano i dati / i messaggi corretti in primo luogo, seguendo i livelli dell'architettura, in modo che i messaggi ti aiutino a identificare un problema che potrebbe verificarsi.

    
risposta data 29.12.2015 - 21:08
fonte
1

Penso che il problema sia a un livello più basilare: si registra l'errore e lo si lancia come eccezione nello stesso posto. Questo è l'anti-modello. Ciò significa che lo stesso errore viene registrato molte volte se viene catturato, magari racchiuso in un'altra eccezione e re-gettato.

Invece di questo suggerisco di registrare gli errori non quando viene creata l'eccezione, ma quando viene catturata. (Per questo, ovviamente, devi assicurarti che venga sempre catturato da qualche parte.) Quando viene rilevata un'eccezione, registro solo la sua stacktrace se è non ri-generata o incapsulata come causa in un'altra eccezione Gli stacktraces e i messaggi delle eccezioni avvolte sono registrati nelle tracce dello stack come "Causato da ...", comunque. E il ricevitore potrebbe anche decidere, ad es. riprovare senza registrare l'errore al primo errore o trattarlo come un avvertimento o altro.

    
risposta data 05.01.2016 - 11:05
fonte
1

So che è un thread vecchio, ma mi sono imbattuto in un problema simile e ho trovato una soluzione simile, quindi aggiungerò i miei 2 centesimi.

Non compro l'argomento della violazione SRP. Non del tutto comunque. Assumiamo 2 cose: 1. Effettivamente si desidera registrare le eccezioni non appena si verificano (a livello di traccia per poter ricreare il flusso del programma). Questo non ha nulla a che fare con la gestione delle eccezioni. 2. Non puoi o non utilizzerai AOP per questo - sono d'accordo che sarebbe il modo migliore per andare ma sfortunatamente sono bloccato con un linguaggio che non fornisce strumenti per questo.

Per come la vedo io, in pratica sei condannato a una violazione SRP su larga scala, perché qualsiasi classe che voglia generare eccezioni deve essere a conoscenza del log. Lo spostamento della registrazione nella classe di eccezioni riduce notevolmente la violazione SRP perché ora solo l'eccezione lo viola e non tutte le classi nel codice di base.

    
risposta data 15.09.2018 - 11:32
fonte
0

Questo è un anti-pattern.

A mio parere, effettuare una chiamata di registrazione nel costruttore di un'eccezione sarebbe un esempio di quanto segue: Flaw: Constructor esegue Real Work .

Non mi aspetterei mai (o desidererei) un costruttore di effettuare qualche chiamata di servizio esterna. Questo è un effetto collaterale molto indesiderabile che, come sottolinea Miško Hevery, costringe sottoclassi e mocker ad ereditare comportamenti indesiderati.

Pertanto, violerebbe anche il principio di minimo stupore .

Se stai sviluppando un'applicazione con altri, questo effetto collaterale probabilmente non sarà evidente a loro. Anche se lavori da solo, potresti dimenticartene e sorprenderti lungo la strada.

    
risposta data 15.09.2018 - 20:45
fonte

Leggi altre domande sui tag