Devo registrar erros em exceções que lançam construtores?

Eu estava construindo um aplicativo por alguns meses e percebi um padrão que surgiu:

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

Ou, ao capturar:

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

Portanto, sempre que eu estava lançando ou capturando uma exceção, eu a registrava. Na verdade, isso foi quase todo o log que fiz no aplicativo (além de algo para inicialização do aplicativo).

Então, como programador, evito a repetição; Por isso, decidi mover as chamadas do criador de logs para a construção da exceção, portanto, sempre que eu estivesse criando uma exceção, as coisas seriam registradas. Eu também poderia, certamente, criar um ExceptionHelper que lançou a exceção para mim, mas isso tornaria meu código mais difícil de interpretar e, ainda pior, o compilador não lidaria bem com ele, deixando de perceber que uma chamada para esse membro jogue imediatamente.

Então, isso é um anti-padrão? Se sim, por quê?

Não tenho certeza se ele se qualifica como antipadrão, mas a IMO é uma péssima idéia: é um acoplamento desnecessário para entrelaçar uma exceção com o log.

Nem sempre você deseja registrar todas as instâncias de uma determinada exceção (talvez isso ocorra durante a validação de entrada, cujo registro pode ser volumoso e desinteressante).

Em segundo lugar, você pode decidir registrar diferentes ocorrências de um erro com diferentes níveis de registro. Nesse momento, é necessário especificar isso ao construir a exceção, o que novamente representa confundir a criação da exceção com o comportamento do registro.

Finalmente, e se ocorrerem exceções durante o registro de outra exceção? Você registraria isso? Fica bagunçado ...

Suas escolhas são basicamente:

• Pegar, registrar e (re) jogar, como você deu no seu exemplo
• Crie uma classe ExceptionHelper para fazer as duas coisas por você, mas os Helpers têm um cheiro de código e eu também não recomendaria isso.
• Mova a manipulação de exceção abrangente para um nível superior
• Considere o AOP para uma solução mais sofisticada para preocupações transversais, como registro e manipulação de exceções (mas muito mais complexa do que apenas ter essas duas linhas nos blocos de captura;))

Então, como programador, evito repetições [...]

Há um perigo aqui sempre que o conceito de "don't repeat yourself"é levado um pouco a sério demais a ponto de se tornar o cheiro.

Para ecoar @ Dan1701

Trata-se da separação de preocupações - mover o log para a exceção cria um forte acoplamento entre a exceção e o log e também significa que você adicionou uma responsabilidade adicional à exceção do log que, por sua vez, pode criar dependências para a classe de exceção que não precisa.

Do ponto de vista da manutenção, você pode (eu diria) que está escondendo o fato de que a exceção está sendo registrada do mantenedor (pelo menos no contexto de algo como o exemplo), também está alterando o contexto ( do local do manipulador de exceções ao construtor da exceção), que provavelmente não é o que você pretendia.

Finalmente, você está assumindo que sempre deseja registrar uma exceção, exatamente da mesma maneira, no momento em que ela é criada / gerada - o que provavelmente não é o caso. A menos que você tenha exceções de registro e não registro, que seriam profundamente desagradáveis ​​rapidamente.

Então, neste caso, acho que "SRP" supera "DRY".

Seu erro está registrando uma exceção em que você não pode manipulá-lo e, portanto, não pode saber se o log faz parte do tratamento adequado.
Existem muito poucos casos em que você pode ou deve lidar com parte do erro, que pode incluir o log, mas ainda deve sinalizar o erro para o chamador. Exemplos são erros de leitura incorrigíveis e similares, se você fizer a leitura de nível mais baixo, mas eles geralmente têm em comum que as informações comunicadas ao chamador são severamente filtradas, para segurança e usabilidade.

A única coisa que você pode fazer no seu caso, e por alguma razão precisa fazer, é traduzir a exceção que a implementação lança em uma que o chamador espera, encadear o original para o contexto e deixar qualquer outra coisa sozinha.

Para resumir, seu código arrogou direitos e deveres para o tratamento parcial da exceção, violando o SRP.
DRY não entra nele.

Reconfigurar uma exceção apenas porque você decidiu registrá-la usando um bloco catch (o que significa que a exceção não foi alterada) é uma má idéia.

Uma das razões pelas quais usamos exceções, mensagens de exceção e seu tratamento é para que saibamos o que deu errado e as exceções escritas com inteligência podem acelerar a localização do bug por uma grande margem.

Lembre-se também de que lidar com exceções custa muito mais recursos do que, digamos, ter if, portanto, você não deve lidar com todos com frequência, apenas porque lhe apetecer. Isso afeta o desempenho do seu aplicativo.

No entanto, é uma boa abordagem usar a exceção como um meio para marcar a camada de aplicativo na qual o erro apareceu.

Considere o seguinte código 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
    }
}

Vamos supor que alguém tenha chamado GetNumberAndReturnCodee recebido o 500código, sinalizando um erro. Ele ligaria para o suporte, que abriria o arquivo de log e veria isso:

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

O desenvolvedor então sabe imediatamente qual camada do software interrompeu o processo e tem uma maneira fácil de identificar o problema. Nesse caso, é crítico, porque o tempo limite do Redis nunca deve ocorrer.

Talvez outro usuário chame o mesmo método, também receba o 500código, mas o log mostre o seguinte:

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.

Nesse caso, o suporte pode simplesmente responder ao usuário que a solicitação é inválida porque ele está solicitando um valor para um ID inexistente.

Sumário

Se você estiver lidando com exceções, certifique-se de lidar com elas da maneira correta. Verifique também se suas exceções incluem os dados / mensagens corretos, seguindo as camadas da arquitetura, para que as mensagens o ajudem a identificar um problema que possa ocorrer.

Acho que o problema está em um nível mais básico: você registra o erro e o lança como uma exceção no mesmo local. Esse é o anti-padrão. Isso significa que o mesmo erro é registrado muitas vezes se for capturado, talvez envolvido em outra exceção e novamente reproduzido.

Em vez disso, sugiro registrar erros não quando a exceção é criada, mas quando é capturada. (Para isso, é claro, você deve fazer em algum lugar se ele está sempre me chamou.) Quando uma exceção é detectada, eu só logar sua stacktrace se ele é não re-lançada ou enrolado como causa para outra exceção. Os rastreamentos de pilha e as mensagens de exceções agrupadas são registrados nos rastreamentos de pilha como "Causados ​​por ...", de qualquer maneira. E o apanhador também pode decidir, por exemplo, tentar novamente sem registrar o erro na primeira falha, ou apenas tratá-lo como um aviso ou qualquer outra coisa.

Eu sei que é um tópico antigo, mas me deparei com um problema semelhante e descobri uma solução semelhante, então adicionarei meus 2 centavos.

Eu não compro o argumento de violação do SRP. Não totalmente de qualquer maneira. Vamos supor duas coisas: 1. Você realmente deseja registrar exceções à medida que elas ocorrem (no nível de rastreio para poder recriar o fluxo do programa). Isso não tem nada a ver com o tratamento de exceções. 2. Você não pode ou não usará o AOP para isso - concordo que seria o melhor caminho a seguir, mas infelizmente estou preso a uma linguagem que não fornece ferramentas para isso.

Na minha opinião, você está basicamente condenado a uma violação de SRP em larga escala, porque qualquer classe que deseja lançar exceções precisa estar ciente do log. Mover o log para a classe de exceção na verdade reduz bastante a violação SRP, porque agora apenas a exceção a viola e nem todas as classes na base de código.

Este é um anti-padrão.

Na minha opinião, fazer uma chamada de log no construtor de uma exceção seria um exemplo do seguinte: Falha: Construtor faz um trabalho real .

Eu nunca esperaria (ou desejaria) que um construtor fizesse alguma chamada de serviço externa. Esse é um efeito colateral indesejável que, como Miško Hevery salienta, força subclasses e zombarias a herdar comportamentos indesejados.

Como tal, também violaria o princípio de menor espanto .

Se você estiver desenvolvendo um aplicativo com outras pessoas, esse efeito colateral provavelmente não será aparente para elas. Mesmo se você estiver trabalhando sozinho, você pode esquecer e se surpreender no caminho.