Dois-je enregistrer des erreurs sur les constructeurs de levée d'exceptions?

Je construisais une application depuis quelques mois et j'en viens à réaliser un schéma qui s'est dégagé:

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

Ou, lors de la capture:

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

Donc, chaque fois que je lançais ou attrapais une exception, je la consignais. En fait, c'était presque toute la journalisation que j'ai faite sur l'application (en plus de quelque chose pour l'initialisation de l'application).

Donc, en tant que programmeur, j'évite la répétition; J'ai donc décidé de déplacer les appels de l'enregistreur vers la construction d'exception, donc, chaque fois que je construisais une exception, les choses étaient enregistrées. Je pourrais aussi, certainement, créer un ExceptionHelper qui a levé l'exception pour moi, mais cela rendrait mon code plus difficile à interpréter et, pire encore, le compilateur ne le traiterait pas bien, ne réalisant pas qu'un appel à ce membre serait jeter immédiatement.

Alors, est-ce un anti-modèle? Si oui, pourquoi?

Je ne sais pas si cela peut être considéré comme un anti-modèle, mais IMO c'est une mauvaise idée: c'est un couplage inutile pour entrelacer une exception avec la journalisation.

Il se peut que vous ne souhaitiez pas toujours enregistrer toutes les instances d'une exception donnée (cela peut se produire lors de la validation des entrées, dont la journalisation peut être à la fois volumineuse et sans intérêt).

Deuxièmement, vous pouvez décider de consigner différentes occurrences d'une erreur avec différents niveaux de journalisation, à quel moment vous devrez le spécifier lors de la construction de l'exception, ce qui représente à nouveau un brouillage de la création de l'exception avec un comportement de journalisation.

Enfin, que se passe-t-il si des exceptions se produisent lors de la consignation d'une autre exception? Souhaitez-vous enregistrer cela? Ça devient désordonné ...

Vos choix sont essentiellement:

• Attrapez, connectez et (re) lancez, comme vous l'avez donné dans votre exemple
• Créez une classe ExceptionHelper pour faire les deux pour vous, mais les Helpers ont une odeur de code, et je ne le recommanderais pas non plus.
• Déplacer la gestion des exceptions fourre-tout à un niveau supérieur
• Envisagez AOP pour une solution plus sophistiquée aux problèmes transversaux tels que la journalisation et la gestion des exceptions (mais beaucoup plus complexe que d'avoir simplement ces deux lignes dans vos blocs de capture;))

Donc, en tant que programmeur, j'évite la répétition [...]

Il y a un danger ici chaque fois que le concept de "don't repeat yourself"est pris un peu trop au sérieux au point de devenir l'odeur.

Pour faire écho @ Dan1701

Il s'agit de la séparation des préoccupations - le déplacement de la journalisation dans l'exception crée un couplage étroit entre l'exception et la journalisation et signifie également que vous avez ajouté une responsabilité supplémentaire à l'exception de la journalisation, qui à son tour peut créer des dépendances pour la classe d'exception qu'elle n'a pas besoin.

Du point de vue de la maintenance, vous pouvez (je dirais) prétendre que vous cachez le fait que l'exception est consignée auprès du responsable (au moins dans le contexte de quelque chose comme l'exemple), vous changez également le contexte ( de l'emplacement du gestionnaire d'exceptions au constructeur de l'exception), ce qui n'est probablement pas ce que vous vouliez.

Enfin, vous faites l'hypothèse que vous voulez toujours enregistrer une exception, exactement de la même manière, au point où elle est créée / levée - ce qui n'est probablement pas le cas. Sauf si vous allez avoir des exceptions de journalisation et de non-journalisation qui deviendraient profondément désagréables assez rapidement.

Donc, dans ce cas, je pense que "SRP" l'emporte sur "DRY".

Votre erreur enregistre une exception lorsque vous ne pouvez pas la gérer et ne peut donc pas savoir si la journalisation fait partie de sa gestion correcte.
Il y a très peu de cas où vous pouvez ou devez gérer une partie de l'erreur, ce qui peut inclure la journalisation, mais doit toujours signaler l'erreur à l'appelant. Des exemples sont des erreurs de lecture non corrigibles et similaires, si vous effectuez la lecture de niveau le plus bas, mais ils ont généralement en commun que les informations communiquées à l'appelant sont sévèrement filtrées, pour des raisons de sécurité et de convivialité.

La seule chose que vous pouvez faire dans votre cas, et pour une raison que vous devez faire, est de traduire l'exception que l'implémentation jette dans celle attendue par l'appelant, de chaîner l'original pour le contexte et de laisser tout le reste bien seul.

Pour résumer, votre code a arrogé le droit et le devoir de traitement partiel de l'exception, violant ainsi le SRP.
SEC n'y entre pas.

Renvoyer une exception uniquement parce que vous avez décidé de l'enregistrer à l'aide d'un bloc catch (ce qui signifie que l'exception n'a pas changé du tout) est une mauvaise idée.

L'une des raisons pour lesquelles nous utilisons les exceptions, les messages d'exceptions et leur gestion est pour que nous sachions ce qui s'est mal passé et les exceptions intelligemment écrites peuvent accélérer la recherche du bogue par une grande marge.

Rappelez-vous également que la gestion des exceptions coûte beaucoup plus de ressources que disons une if, donc vous ne devriez pas toutes les gérer trop souvent simplement parce que vous en avez envie. Cela a un impact sur les performances de votre application.

Il est toutefois judicieux d'utiliser l'exception comme moyen de marquer la couche d'application dans laquelle l'erreur est apparue.

Considérez le semi-pseudo-code suivant:

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
    }
}

Supposons que quelqu'un ait appelé GetNumberAndReturnCodeet reçu le 500code, signalant une erreur. Il appelait le support, qui ouvrait le fichier journal et voyait ceci:

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

Le développeur sait alors immédiatement quelle couche du logiciel a provoqué l'abandon du processus et a un moyen facile d'identifier le problème. Dans ce cas, c'est critique, car le délai d'expiration de Redis ne devrait jamais arriver.

Peut-être qu'un autre utilisateur appellerait la même méthode, recevrait également le 500code, mais le journal afficherait ce qui suit:

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.

Dans ce cas, le support pourrait simplement répondre à l'utilisateur que la demande n'était pas valide car il demande une valeur pour un ID inexistant.

Sommaire

Si vous gérez des exceptions, assurez-vous de les gérer correctement. Assurez-vous également que vos exceptions incluent les données / messages corrects en premier lieu, en suivant vos couches d'architecture, afin que les messages vous aident à identifier un problème qui peut se produire.

Je pense que le problème est à un niveau plus basique: vous enregistrez l'erreur et la jetez en tant qu'exception au même endroit. Voilà l'anti-modèle. Cela signifie que la même erreur est enregistrée plusieurs fois si elle est interceptée, peut-être enveloppée dans une autre exception et renvoyée.

Au lieu de cela, je suggère de consigner les erreurs non pas lorsque l'exception est créée, mais lorsqu'elle est détectée. (Pour cela, bien sûr, vous devez vous assurer qu'elle est toujours interceptée quelque part.) Lorsqu'une exception est interceptée, je n'enregistre sa trace de pile que si elle n'est pas renvoyée ou encapsulée comme cause dans une autre exception. Les traces de pile et les messages des exceptions encapsulées sont de toute façon enregistrés dans les traces de pile comme "Causé par ...". Et le receveur pourrait également décider, par exemple, de réessayer sans enregistrer l'erreur sur le premier échec, ou simplement la traiter comme un avertissement, ou autre.

Je sais que c'est un vieux fil, mais je viens de rencontrer un problème similaire et j'ai trouvé une solution similaire, donc je vais ajouter mes 2 cents.

Je n'achète pas l'argument de violation SRP. Pas entièrement de toute façon. Supposons 2 choses: 1. Vous voulez réellement enregistrer les exceptions au fur et à mesure qu'elles se produisent (au niveau de la trace pour pouvoir recréer le flux du programme). Cela n'a rien à voir avec la gestion des exceptions. 2. Vous ne pouvez pas ou vous n'utiliserez pas AOP pour cela - je suis d'accord que ce serait la meilleure façon de procéder, mais malheureusement je suis coincé avec un langage qui ne fournit pas d'outils pour cela.

À mon avis, vous êtes essentiellement condamné à une violation SRP à grande échelle, car toute classe qui souhaite lever des exceptions doit être au courant du journal. Le déplacement de la journalisation vers la classe d'exception réduit en fait considérablement la violation SRP car maintenant seule l'exception la viole et pas toutes les classes de la base de code.

Ceci est un anti-modèle.

À mon avis, effectuer un appel de journalisation dans le constructeur d'une exception serait un exemple de ce qui suit: Défaut: le constructeur fait du vrai travail .

Je ne m'attendrais jamais (ou ne souhaiterais) pas qu'un constructeur fasse un appel de service externe. C'est un effet secondaire très indésirable qui, comme le souligne Miško Hevery, oblige les sous-classes et les simulacres à hériter d'un comportement indésirable.

En tant que tel, il violerait également le principe du moindre étonnement .

Si vous développez une application avec d'autres, cet effet secondaire ne leur sera probablement pas apparent. Même si vous travaillez seul, vous pouvez l'oublier et vous surprendre sur la route.