¿Debo registrar errores en constructores de lanzamiento de excepciones?

Estuve creando una aplicación durante unos meses y me di cuenta de un patrón que surgió:

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

O, al atrapar:

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

Entonces, cada vez que lanzaba o atrapaba una excepción, lo registraba. De hecho, eso fue casi todo el registro que hice en la aplicación (además de algo para la inicialización de la aplicación).

Entonces, como programador, evito la repetición; Así que decidí mover las llamadas del registrador a la construcción de la excepción, por lo que, cada vez que creaba una excepción, las cosas se registraban. Ciertamente, también podría crear un ExceptionHelper que arrojara la excepción para mí, pero eso haría que mi código fuera más difícil de interpretar y, lo que es peor, el compilador no lo trataría bien, al no darse cuenta de que una llamada a ese miembro tirar de inmediato.

Entonces, ¿es esto un antipatrón? Si es así, ¿por qué?

No estoy seguro si califica como un antipatrón, pero IMO es una mala idea: es un acoplamiento innecesario para entrelazar una excepción con el registro.

Es posible que no siempre desee registrar todas las instancias de una excepción dada (tal vez ocurra durante la validación de entrada, cuyo registro puede ser voluminoso y poco interesante).

En segundo lugar, puede decidir registrar diferentes ocurrencias de un error con diferentes niveles de registro, momento en el que tendría que especificar eso al construir la excepción, lo que nuevamente representa enturbiar la creación de excepciones con el comportamiento de registro.

Finalmente, ¿qué pasa si se produjeron excepciones durante el registro de otra excepción? ¿Registrarías eso? Se pone desordenado ...

Sus elecciones son básicamente:

• Atrapa, registra y (re) lanza, como dijiste en tu ejemplo
• Cree una clase ExceptionHelper para hacer las dos cosas por usted, pero los Helpers tienen un olor a código, y tampoco lo recomendaría.
• Mueva el manejo de excepciones generales a un nivel superior
• Considere AOP para obtener una solución más sofisticada para problemas transversales como el registro y el manejo de excepciones (pero mucho más complejo que solo tener esas dos líneas en sus bloques de captura;))

Entonces, como programador, evito la repetición [...]

Existe un peligro aquí cuando el concepto de "don't repeat yourself"se toma demasiado en serio hasta el punto de convertirse en olor.

Hacer eco @ Dan1701

Esto se trata de la separación de preocupaciones: mover el registro a la excepción crea un acoplamiento estrecho entre la excepción y el registro y también significa que ha agregado una responsabilidad adicional a la excepción del registro que a su vez puede crear dependencias para la clase de excepción que no necesita

Desde el punto de vista del mantenimiento, puede (yo diría) que está ocultando el hecho de que la excepción se está registrando desde el mantenedor (al menos en el contexto de algo como el ejemplo), también está cambiando el contexto ( desde la ubicación del controlador de excepciones hasta el constructor de la excepción), que probablemente no sea lo que pretendía.

Finalmente, está asumiendo que siempre desea registrar una excepción, exactamente de la misma manera, en el punto en el que se creó / provocó, lo que probablemente no sea el caso. A menos que tenga excepciones de registro y no registro que se volverían profundamente desagradables con bastante rapidez.

Entonces, en este caso, creo que "SRP" triunfa sobre "DRY".

Su error es registrar una excepción donde no puede manejarlo y, por lo tanto, no puede saber si el registro es parte de manejarlo adecuadamente.
Hay muy pocos casos en los que puede o debe manejar parte del error, que puede incluir el registro, pero aún debe indicar el error a la persona que llama. Los ejemplos son errores de lectura no corregibles y similares, si realiza la lectura de nivel más bajo, pero generalmente tienen en común que la información comunicada a la persona que llama se filtra severamente, por seguridad y usabilidad.

Lo único que puede hacer en su caso, y por alguna razón tiene que hacer, es traducir la excepción que la implementación arroja en una que el llamante espera, encadenar el contexto original y dejar todo lo demás en paz.

Para resumir, su código arrojó derechos y deberes para el manejo parcial de la excepción, violando así el SRP.
DRY no entra en eso.

Volver a lanzar una excepción solo porque decidió iniciar sesión utilizando un bloque catch (lo que significa que la excepción no ha cambiado en absoluto) es una mala idea.

Una de las razones por las que usamos excepciones, mensajes de excepciones y su manejo es para que sepamos qué salió mal y las excepciones escritas inteligentemente pueden acelerar la búsqueda del error por un gran margen.

También recuerde, manejar excepciones cuesta mucho más recursos que, digamos, tener un if , por lo que no debería manejarlos todos a menudo solo porque lo desee. Tiene impacto en el rendimiento de su aplicación.

Sin embargo, es un buen enfoque utilizar la excepción como un medio para marcar la capa de aplicación en la que apareció el error.

Considere el siguiente semi-pseudocódigo:

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

Supongamos que alguien ha llamado GetNumberAndReturnCodey recibido el 500código, señalando un error. Llamaría al soporte, quien abriría el archivo de registro y vería esto:

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

Luego, el desarrollador sabe de inmediato qué capa del software provocó la interrupción del proceso y tiene una manera fácil de identificar el problema. En este caso es crítico, porque el tiempo de espera de Redis nunca debería suceder.

Quizás otro usuario llame al mismo método, también reciba el 500código, pero log mostrará lo siguiente:

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.

En cuyo caso, el soporte podría simplemente responder al usuario que la solicitud no era válida porque está solicitando un valor para una ID inexistente.

Resumen

Si está manejando excepciones, asegúrese de manejarlas de la manera correcta. También asegúrese de que sus excepciones incluyan los datos / mensajes correctos en primer lugar, siguiendo sus capas de arquitectura, para que los mensajes lo ayuden a identificar un problema que pueda ocurrir.

Creo que el problema está en un nivel más básico: registra el error y lo arroja como una excepción en el mismo lugar. Ese es el antipatrón. Esto significa que el mismo error se registra muchas veces si se detecta, quizás envuelto en otra excepción y se vuelve a lanzar.

En lugar de esto, sugiero registrar los errores no cuando se crea la excepción, sino cuando se detecta. (Para esto, por supuesto, usted debe hacer algún lugar seguro de que siempre está atrapado.) Cuando se detecta una excepción, que sólo había conecto su StackTrace si es no volver a arrojar o envuelto como causa en otra excepción. Los seguimientos de pila y los mensajes de excepciones envueltas se registran en seguimientos de pila como "Causados ​​por ...", de todos modos. Y el receptor también podría decidir, por ejemplo, volver a intentarlo sin registrar el error en la primera falla, o simplemente tratarlo como una advertencia, o lo que sea.

Sé que es un hilo viejo, pero me encontré con un problema similar y descubrí una solución similar, así que agregaré mis 2 centavos.

No compro el argumento de violación de SRP. No del todo de todos modos. Supongamos 2 cosas: 1. Realmente desea registrar las excepciones a medida que ocurren (a nivel de rastreo para poder recrear el flujo del programa). Esto no tiene nada que ver con el manejo de excepciones. 2. No puedes o no usarás AOP para eso. Estoy de acuerdo en que sería la mejor manera de hacerlo, pero desafortunadamente estoy atrapado en un lenguaje que no proporciona herramientas para eso.

A mi modo de ver, básicamente estás condenado a una violación de SRP a gran escala, porque cualquier clase que quiera lanzar excepciones tiene que tener en cuenta el registro. Mover el registro a la clase de excepción en realidad reduce en gran medida la violación de SRP porque ahora solo la excepción lo viola y no todas las clases en la base del código.

Este es un antipatrón.

En mi opinión, hacer una llamada de registro en el constructor de una excepción sería un ejemplo de lo siguiente: Defecto: el constructor hace un trabajo real .

Nunca esperaría (o desearía) que un constructor haga alguna llamada de servicio externo. Ese es un efecto secundario muy indeseable que, como señala Miško Hevery, obliga a las subclases y simulacros a heredar comportamientos no deseados.

Como tal, también violaría el principio del menor asombro .

Si está desarrollando una aplicación con otros, entonces este efecto secundario probablemente no será evidente para ellos. Incluso si está trabajando solo, puede olvidarse de eso y sorprenderse en el futuro.