Sollte ich Fehler bei Konstruktoren protokollieren, die Ausnahmen auslösen?

Ich habe einige Monate lang eine Anwendung erstellt und dabei ein Muster erkannt, das sich herausstellte:

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

Oder beim Fangen:

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

Wenn ich also eine Ausnahme ausgelöst oder abgefangen habe, habe ich sie protokolliert. Tatsächlich war das fast die gesamte Protokollierung, die ich für die Anwendung durchgeführt habe (abgesehen von etwas für die Anwendungsinitialisierung).

Als Programmierer vermeide ich Wiederholungen. Deshalb habe ich mich entschlossen, die Logger-Aufrufe auf die Exception-Konstruktion zu verschieben. Wenn ich also eine Exception baue, werden die Dinge protokolliert. Ich könnte natürlich auch einen ExceptionHelper erstellen, der die Ausnahme für mich auslöst, aber das würde die Interpretation meines Codes erschweren und, noch schlimmer, der Compiler würde nicht gut damit umgehen, da er nicht erkennt, dass ein Aufruf an dieses Mitglied zu erwarten wäre sofort werfen.

Also, ist das ein Anti-Muster? Wenn ja warum?

Ich bin nicht sicher, ob es sich um ein Anti-Pattern handelt, aber IMO ist eine schlechte Idee: Es ist unnötig, eine Ausnahme mit der Protokollierung zu verknüpfen.

Möglicherweise möchten Sie nicht immer alle Instanzen einer bestimmten Ausnahme protokollieren (möglicherweise tritt sie während der Eingabevalidierung auf, deren Protokollierung sowohl umfangreich als auch uninteressant sein kann).

Zweitens können Sie sich dafür entscheiden, unterschiedliche Fehlerereignisse mit unterschiedlichen Protokollierungsstufen zu protokollieren. Zu diesem Zeitpunkt müssen Sie dies beim Erstellen der Ausnahme angeben, was wiederum bedeutet, dass die Erstellung der Ausnahme durch das Protokollierungsverhalten beeinträchtigt wird.

Was ist, wenn während der Protokollierung einer anderen Ausnahme Ausnahmen aufgetreten sind? Würden Sie das protokollieren? Es wird chaotisch ...

Ihre Wahlmöglichkeiten sind grundsätzlich:

• Fangen, protokollieren und (erneut) werfen, wie Sie es in Ihrem Beispiel angegeben haben
• Erstellen Sie eine ExceptionHelper-Klasse, um beides für Sie zu erledigen, aber Helfer haben einen Codegeruch, und ich würde dies auch nicht empfehlen.
• Verschieben Sie die Behandlung von Catch-All-Ausnahmen auf eine höhere Ebene
• Betrachten Sie AOP als eine ausgefeiltere Lösung für übergreifende Probleme wie Protokollierung und Ausnahmebehandlung.

Als Programmierer meide ich also Wiederholungen [...]

Es besteht hier die Gefahr, wenn der Begriff „ "don't repeat yourself"ein wenig zu ernst genommen wird, bis er zum Geruch wird.

@ Dan1701 zu echo

Hier geht es um die Trennung von Bedenken: Durch das Verschieben der Protokollierung in die Ausnahme wird eine enge Kopplung zwischen der Ausnahme und der Protokollierung hergestellt. Außerdem haben Sie der Ausnahme der Protokollierung eine zusätzliche Verantwortung hinzugefügt, die Abhängigkeiten für die Ausnahmeklasse verursachen kann braucht nicht.

Unter dem Gesichtspunkt der Wartung können Sie (ich würde) argumentieren, dass Sie die Tatsache verbergen, dass die Ausnahme vom Betreuer protokolliert wird (zumindest im Kontext des Beispiels). Sie ändern auch den Kontext ( vom Ort des Ausnahmebehandlers zum Konstruktor der Ausnahme), was wahrscheinlich nicht das ist , was Sie beabsichtigt haben.

Schließlich gehen Sie davon aus, dass Sie eine Ausnahme immer genau so protokollieren möchten, an dem Punkt, an dem sie erstellt / ausgelöst wird - was wahrscheinlich nicht der Fall ist. Es sei denn, Sie werden protokollierte und nicht protokollierte Ausnahmen haben, die sehr schnell sehr unangenehm werden würden.

Also in diesem Fall denke ich, dass "SRP" "DRY" übertrifft.

Ihr Fehler protokolliert eine Ausnahme, bei der Sie nicht damit umgehen können und daher nicht wissen können, ob die Protokollierung ein Teil der ordnungsgemäßen Behandlung ist.
Es gibt sehr wenige Fälle, in denen Sie einen Teil des Fehlers behandeln können oder müssen, einschließlich der Protokollierung, aber den Fehler trotzdem dem Anrufer melden müssen. Beispiele sind nicht korrigierbare Lesefehler und dergleichen, wenn Sie auf der untersten Ebene lesen, aber sie haben im Allgemeinen gemeinsam, dass die an den Anrufer übermittelten Informationen aus Sicherheits- und Verwendbarkeitsgründen stark gefiltert sind.

Das einzige, was Sie in Ihrem Fall tun können und aus irgendeinem Grund tun müssen, ist, die Ausnahme, die die Implementierung auslöst, in eine vom Aufrufer erwartete zu übersetzen, das Original für den Kontext zu verketten und alles andere in Ruhe zu lassen.

Zusammenfassend lässt sich sagen, dass Ihr Code das Recht und die Pflicht zur teilweisen Behandlung der Ausnahme verletzt und damit die SRP verletzt hat.
DRY kommt nicht rein.

Das erneute Auslösen einer Ausnahme nur, weil Sie beschlossen haben, sie mit einem catch-Block zu protokollieren (was bedeutet, dass sich die Ausnahme überhaupt nicht geändert hat), ist eine schlechte Idee.

Einer der Gründe, warum wir Ausnahmen, Ausnahmemeldungen und deren Behandlung verwenden, ist, dass wir wissen, was schief gelaufen ist, und dass geschickt geschriebene Ausnahmen das Auffinden des Fehlers erheblich beschleunigen können.

Denken Sie auch daran, dass das Behandeln von Ausnahmen weitaus mehr Ressourcen kostet, als wir vielleicht haben. ifSie sollten sie also nicht allzu oft behandeln, nur weil Sie Lust dazu haben. Dies hat Auswirkungen auf die Leistung Ihrer Anwendung.

Es ist jedoch ein guter Ansatz, Ausnahme als Mittelwert zu verwenden, um die Anwendungsschicht zu markieren, in der der Fehler aufgetreten ist.

Betrachten Sie den folgenden Halbpseudocode:

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

Nehmen wir an, jemand hat den angerufen GetNumberAndReturnCodeund den 500Code erhalten, was auf einen Fehler hinweist . Er würde den Support anrufen, der die Protokolldatei öffnen und Folgendes sehen würde:

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

Der Entwickler weiß dann sofort, auf welcher Ebene der Software der Prozess abgebrochen wurde, und kann das Problem auf einfache Weise identifizieren. In diesem Fall ist es wichtig, da Redis Timeout niemals eintreten sollte.

Möglicherweise würde ein anderer Benutzer dieselbe Methode aufrufen und auch den 500Code erhalten, das Protokoll würde jedoch Folgendes anzeigen :

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 diesem Fall kann der Support dem Benutzer einfach antworten, dass die Anforderung ungültig ist, weil er einen Wert für eine nicht vorhandene ID anfordert.

Zusammenfassung

Wenn Sie mit Ausnahmen umgehen, stellen Sie sicher, dass Sie sie richtig behandeln. Stellen Sie außerdem sicher, dass Ihre Ausnahmen in erster Linie die richtigen Daten / Nachrichten enthalten, die Ihren Architekturebenen folgen, damit Sie anhand der Nachrichten ein Problem identifizieren können, das möglicherweise auftritt.

Ich denke, das Problem liegt auf einer grundlegenderen Ebene: Sie protokollieren den Fehler und werfen ihn als Ausnahme an die gleiche Stelle. Das ist das Anti-Muster. Dies bedeutet, dass derselbe Fehler viele Male protokolliert wird, wenn er abgefangen, möglicherweise in eine andere Ausnahme eingeschlossen und erneut ausgelöst wird.

Stattdessen empfehle ich, Fehler nicht zu protokollieren, wenn die Ausnahme erstellt wird, sondern wenn sie abgefangen wird. (Dafür müssen Sie natürlich sicherstellen, dass es immer irgendwo abgefangen wird.) Wenn eine Ausnahme abgefangen wird, würde ich den Stacktrace nur dann protokollieren, wenn es nicht erneut geworfen oder als Ursache in eine andere Ausnahme eingeschlossen wird. Die Stacktraces und Meldungen von umbrochenen Ausnahmen werden in Stacktraces sowieso als "Caused by ..." protokolliert. Und der Fänger könnte sich auch dafür entscheiden, es erneut zu versuchen, ohne den Fehler beim ersten Fehler aufzuzeichnen, oder ihn einfach als Warnung zu behandeln, oder was auch immer.

Ich weiß, es ist ein alter Thread, aber ich bin gerade auf ein ähnliches Problem gestoßen und habe eine ähnliche Lösung gefunden, also zähle ich meine 2 Cent dazu.

Ich kaufe das Argument der SRP-Verletzung nicht. Jedenfalls nicht ganz. Nehmen wir zwei Dinge an: 1. Sie möchten tatsächlich Ausnahmen protokollieren, sobald sie auftreten (auf Ablaufverfolgungsebene, um den Programmfluss neu erstellen zu können). Dies hat nichts mit der Behandlung von Ausnahmen zu tun. 2. Sie können oder werden AOP dafür nicht verwenden - ich stimme zu, dass dies der beste Weg wäre, aber leider stecke ich in einer Sprache fest, die keine Tools dafür bietet.

So wie ich das sehe, sind Sie im Grunde genommen zu einer großen SRP-Verletzung verurteilt, weil jede Klasse, die Ausnahmen auslösen möchte, das Protokoll kennen muss. Durch das Verschieben der Protokollierung in die Ausnahmeklasse wird die SRP-Verletzung erheblich reduziert, da jetzt nur die Ausnahme und nicht jede Klasse in der Codebasis gegen sie verstößt.

Dies ist ein Anti-Muster.

Meiner Meinung nach wäre ein Protokollierungsaufruf im Konstruktor einer Ausnahme ein Beispiel für Folgendes: Fehler: Der Konstruktor erledigt die eigentliche Arbeit .

Ich würde niemals erwarten (oder wünschen), dass ein Konstruktor einen externen Serviceabruf ausführt. Dies ist eine sehr unerwünschte Nebenwirkung, die, wie Miško Hevery betont, Unterklassen und Spötter dazu zwingt, unerwünschtes Verhalten zu erben.

Als solches würde es auch das Prinzip des geringsten Erstaunens verletzen .

Wenn Sie eine Anwendung mit anderen entwickeln, ist dieser Nebeneffekt für sie wahrscheinlich nicht erkennbar. Selbst wenn Sie alleine arbeiten, können Sie das vergessen und sich selbst überraschen.