Должен ли я регистрировать ошибки при создании исключений?

Я строил приложение в течение нескольких месяцев, и я понял, что возникла закономерность:

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

Или при ловле:

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

Поэтому всякий раз, когда я выбрасывал или ловил исключение, я записывал его. Фактически, это было почти все, что я делал в приложении (кроме чего-то для инициализации приложения).

Итак, как программист, я избегаю повторения; Поэтому я решил переместить вызовы регистратора в конструкцию исключения, поэтому, когда бы я ни создавал исключение, все регистрировалось. Я также мог бы, конечно, создать ExceptionHelper, который бы вызывал для меня исключение, но это усложнило бы интерпретацию моего кода, и, что еще хуже, компилятор не справился бы с этим, не понимая, что вызов этого члена будет брось немедленно.

Так это анти-паттерн? Если так, то почему?

Не уверен, что он считается анти-паттерном, но IMO - плохая идея: это ненужная связь, чтобы переплетать исключение с ведением журнала.

Возможно, вы не всегда хотите регистрировать все экземпляры данного исключения (возможно, это происходит во время проверки входных данных, регистрация которых может быть как объемной, так и неинтересной).

Во-вторых, вы можете решить регистрировать разные случаи возникновения ошибки с разными уровнями ведения журнала, и в этот момент вам нужно будет указать это при создании исключения, что опять же представляет собой затруднение создания исключения с поведением ведения журнала.

Наконец, что если исключения произошли во время регистрации другого исключения? Не могли бы вы войти? Это становится грязным ...

Ваш выбор в основном:

• Поймай, войди и (пере) брось, как ты дал в своем примере
• Создайте класс ExceptionHelper, чтобы сделать оба для вас, но у помощников есть запах кода, и я бы не рекомендовал это тоже.
• Переместить всеобъемлющую обработку исключений на более высокий уровень
• Рассмотрим AOP для более сложного решения сквозных задач, таких как ведение журнала и обработка исключений (но гораздо более сложного, чем просто наличие этих двух строк в ваших блоках перехвата;))

Итак, как программист, я избегаю повторения [...]

Здесь возникает опасность всякий раз, когда концепция "don't repeat yourself"воспринимается слишком серьезно, до такой степени, что она становится запахом.

Эхо @ Dan1701

Речь идет о разделении проблем - перемещение регистрации в исключение создает тесную связь между исключением и журналированием, а также означает, что вы добавили дополнительную ответственность к исключению регистрации, что, в свою очередь, может создать зависимости для класса исключения, который он не нужно

С точки зрения обслуживания вы можете (я бы сказал), что вы скрываете тот факт, что исключение регистрируется от сопровождающего (по крайней мере, в контексте чего-то вроде примера), вы также меняете контекст ( от расположения обработчика исключения до конструктора исключения), что, вероятно, не то, что вы хотели.

Наконец, вы предполагаете, что вы всегда хотите регистрировать исключение, точно таким же образом, в момент, когда оно было создано / возбуждено - что, вероятно, не так. Если только у вас не появятся исключения, связанные с регистрацией и отсутствием регистрации, которые могут стать очень неприятными довольно быстро.

Так что в этом случае я думаю, что «SRP» превосходит «DRY».

Ваша ошибка регистрирует исключение, когда вы не можете его обработать, и, следовательно, не можете знать, является ли ведение журнала частью правильной обработки.
В очень редких случаях вы можете или должны обработать часть ошибки, которая может включать в себя ведение журнала, но все же должна сигнализировать об ошибке вызывающей стороне. Примерами являются некорректируемые ошибки чтения и тому подобное, если вы выполняете чтение самого низкого уровня, но обычно они имеют общую информацию о том, что информация, передаваемая вызывающей стороне, строго фильтруется для безопасности и удобства использования.

Единственное, что вы можете сделать в вашем случае, и по какой-то причине это нужно сделать, это преобразовать исключение, которое выдает реализация, в ожидаемое вызывающим, связать оригинал с контекстом и оставить все остальное в покое.

Подводя итог, ваш код присвоил право и обязанность для частичной обработки исключения, таким образом нарушая SRP.
СУХОЙ не входит в это.

Восстановление исключения только потому, что вы решили записать его с помощью блока catch (то есть исключение не изменилось вообще), является плохой идеей.

Одна из причин, по которой мы используем исключения, сообщения об исключениях и их обработку, заключается в том, что мы знаем, что пошло не так, и грамотно написанные исключения могут значительно ускорить поиск ошибки.

Также помните, что обработка исключений требует гораздо больше ресурсов, чем, скажем, есть if, поэтому вам не следует часто их обрабатывать только потому, что вам так хочется. Это влияет на производительность вашего приложения.

Тем не менее, рекомендуется использовать исключение в качестве средства для обозначения прикладного уровня, на котором возникла ошибка.

Рассмотрим следующий полупсевдокод:

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

Давайте предположим, что кто-то вызвал GetNumberAndReturnCodeи получил 500код, сигнализирующий об ошибке. Он позвонит в службу поддержки, который откроет файл журнала и увидит это:

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

Затем разработчик сразу узнает, какой уровень программного обеспечения вызвал прерывание процесса, и может легко определить проблему. В этом случае это критично, потому что тайм-аут Redis никогда не должен произойти.

Возможно, другой пользователь вызовет тот же метод, также 500получит код, но в журнале будет показано следующее:

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.

В этом случае служба поддержки может просто ответить пользователю, что запрос недействителен, потому что он запрашивает значение для несуществующего идентификатора.

Резюме

Если вы обрабатываете исключения, убедитесь, что обрабатываете их правильно. Также убедитесь, что ваши исключения в первую очередь содержат правильные данные / сообщения, следуя уровням вашей архитектуры, чтобы сообщения помогли вам определить проблему, которая может возникнуть.

Я думаю, что проблема на более простом уровне: вы регистрируете ошибку и выбрасываете ее как исключение в том же месте. Это анти-паттерн. Это означает, что одна и та же ошибка регистрируется много раз, если она обнаружена, возможно, заключена в другое исключение и повторно выброшена.

Вместо этого я предлагаю регистрировать ошибки не при создании исключения, а при его обнаружении. (Для этого, конечно, вы должны убедиться, что оно всегда где-то перехватывается.) Когда перехватывается исключение, я регистрирую его трассировку стека только в том случае, если оно не было переброшено или перенесено как причина в другое исключение. Трассировки стека и сообщения обернутых исключений в любом случае записываются в трассировку стека как «Причины ...». И ловушка также может решить, например, повторить попытку, не регистрируя ошибку при первом сбое, или просто обработать ее как предупреждение, или что-то еще.

Я знаю, что это старая ветка, но я только столкнулся с подобной проблемой и нашел подобное решение, поэтому я добавлю свои 2 цента.

Я не покупаю аргумент нарушения SRP. Во всяком случае, не совсем. Давайте предположим 2 вещи: 1. Вы действительно хотите регистрировать исключения по мере их возникновения (на уровне трассировки, чтобы иметь возможность воссоздать поток программы). Это не имеет ничего общего с обработкой исключений. 2. Вы не можете или не будете использовать AOP для этого - я согласен, что это будет лучшим способом, но, к сожалению, я застрял на языке, который не предоставляет инструментов для этого.

На мой взгляд, вы в основном приговорены к крупномасштабному нарушению SRP, потому что любой класс, который хочет генерировать исключения, должен знать о журнале. Перемещение журналирования в класс исключений фактически значительно уменьшает нарушение SRP, поскольку теперь его нарушает только исключение, а не каждый класс в кодовой базе.

Это анти-паттерн.

По моему мнению, выполнение вызова регистрации в конструкторе исключения будет примером следующего: Ошибка: Конструктор выполняет реальную работу .

Я бы никогда не ожидал (или не хотел бы), чтобы конструктор сделал какой-то внешний вызов службы. Это очень нежелательный побочный эффект, который, как указывает Мишко Хевери, заставляет подклассы и издевательства наследовать нежелательное поведение.

Таким образом, это также нарушит принцип наименьшего удивления .

Если вы разрабатываете приложение для других, этот побочный эффект, скорее всего, не будет им очевиден. Даже если вы работаете в одиночку, вы можете забыть об этом и удивить себя в будущем.