Ist das Werfen einer Ausnahme hier ein Anti-Pattern?

33

Ich hatte gerade eine Diskussion über eine Designauswahl nach einer Codeüberprüfung. Ich frage mich, was deine Meinung ist.

Es gibt diese PreferencesKlasse, die ein Bucket für Schlüssel-Wert-Paare ist. Nullwerte sind legal (das ist wichtig). Wir gehen davon aus, dass bestimmte Werte möglicherweise noch nicht gespeichert wurden, und möchten diese Fälle automatisch behandeln, indem wir sie auf Anforderung mit einem vordefinierten Standardwert initialisieren.

Die besprochene Lösung verwendete folgendes Muster (HINWEIS: Dies ist offensichtlich nicht der eigentliche Code - es wird zur Veranschaulichung vereinfacht):

public class Preferences {
    // null values are legal
    private Map<String, String> valuesFromDatabase;

    private static Map<String, String> defaultValues;

    class KeyNotFoundException extends Exception {
    }

    public String getByKey(String key) {
        try {
            return getValueByKey(key);
        } catch (KeyNotFoundException e) {
            String defaultValue = defaultValues.get(key);
            valuesFromDatabase.put(key, defaultvalue);
            return defaultValue;
        }
    }

    private String getValueByKey(String key) throws KeyNotFoundException {
        if (valuesFromDatabase.containsKey(key)) {
            return valuesFromDatabase.get(key);
        } else {
            throw new KeyNotFoundException();
        }
    }
}

Es wurde als Anti-Pattern kritisiert - Missbrauch von Ausnahmen, um den Fluss zu kontrollieren . KeyNotFoundException- nur für diesen einen Anwendungsfall zum Leben erweckt - wird niemals aus dem Rahmen dieser Klasse herausgesehen.

Es sind im Wesentlichen zwei Methoden, die damit spielen, nur um etwas miteinander zu kommunizieren.

Der Schlüssel, der nicht in der Datenbank vorhanden ist, ist nicht alarmierend oder außergewöhnlich. Dies wird erwartet, wenn eine neue Voreinstellung hinzugefügt wird, daher der Mechanismus, der ihn bei Bedarf ordnungsgemäß mit einem Standardwert initialisiert.

Das Gegenargument war, dass getValueByKeydie private Methode, wie sie derzeit definiert ist, keine natürliche Möglichkeit bietet, die öffentliche Methode sowohl über den Wert als auch darüber zu informieren , ob der Schlüssel vorhanden ist. (Wenn dies nicht der Fall ist, muss es hinzugefügt werden, damit der Wert aktualisiert werden kann.)

Die Rückgabe nullwäre zweideutig, da nulles sich um einen vollkommen legalen Wert handelt. Es lässt sich also nicht sagen, ob der Schlüssel nicht vorhanden war oder ob ein Schlüssel vorhanden war null.

getValueByKeyTuple<Boolean, String>Müsste eine Art von a zurückgeben , das Bool auf true setzen, wenn der Schlüssel bereits vorhanden ist, damit wir zwischen (true, null)und unterscheiden können (false, null). (Ein outParameter könnte in C # verwendet werden, aber das ist Java).

Ist das eine schönere Alternative? Ja, Sie müssten eine Klasse für den einmaligen Gebrauch definieren Tuple<Boolean, String>, aber dann werden wir sie los KeyNotFoundException, damit diese Art von Ausgleich entsteht. Wir vermeiden auch den Aufwand für die Behandlung von Ausnahmen, obwohl dies praktisch nicht von Bedeutung ist. Es gibt keine nennenswerten Leistungsaspekte, es handelt sich um eine Client-App und es ist nicht so, als würden Benutzereinstellungen millionenfach pro Sekunde abgerufen.

Eine Variation dieses Ansatzes könnte Guavas verwenden Optional<String>(Guava wird bereits im gesamten Projekt verwendet), anstatt irgendeiner Gewohnheit Tuple<Boolean, String>, und dann können wir zwischen Optional.<String>absent()und "richtig" unterscheiden null. Es fühlt sich jedoch aus Gründen, die leicht zu erkennen sind, immer noch hackisch an - die Einführung von zwei Ebenen der "Nullheit" scheint das Konzept zu missbrauchen, das hinter der Schaffung von Optionals an erster Stelle stand.

Eine andere Möglichkeit wäre, explizit zu prüfen, ob der Schlüssel existiert (fügen Sie eine boolean containsKey(String key)Methode hinzu und rufen Sie sie getValueByKeynur auf, wenn wir bereits bestätigt haben, dass sie existiert).

Schließlich könnte man auch die private Methode einbinden, aber die eigentliche getByKeyist etwas komplexer als mein Codebeispiel, so dass das Einbetten ziemlich unangenehm aussehen würde.

Ich mag hier Haare spalten, aber ich bin neugierig, worauf Sie wetten würden, um in diesem Fall der besten Praxis am nächsten zu sein. Ich habe in den Styleguides von Oracle oder Google keine Antwort gefunden.

Ist die Verwendung von Ausnahmen wie im Codebeispiel ein Anti-Pattern, oder ist es akzeptabel, da Alternativen auch nicht sehr sauber sind? Wenn ja, unter welchen Umständen wäre das in Ordnung? Und umgekehrt?

Konrad Morawski
quelle
1
@ GlenH7 die akzeptierte (und am höchsten bewertete) Antwort fasst zusammen, dass "Sie sie [Ausnahmen] in Fällen verwenden sollten, in denen sie die Fehlerbehandlung mit weniger Code-Unordnung erleichtern". Ich schätze, ich frage hier, ob die Alternativen, die ich aufgelistet habe, als "weniger Code-Unordnung" angesehen werden sollten.
Konrad Morawski
1
Ich habe meine VTC zurückgezogen - Sie fragen nach etwas, das mit etwas zu tun hat, aber nicht mit dem von mir vorgeschlagenen Duplikat.
3
Ich denke, die Frage wird interessanter, wenn sie getValueByKeyauch öffentlich ist.
Doc Brown
2
Zum späteren Nachschlagen haben Sie das Richtige getan. Dies wäre bei der Codeüberprüfung nicht zum Thema geworden, da es sich um Beispielcode handelt.
RubberDuck
1
Nur um sich einzuloggen - das ist absolut idiomatisch für Python und würde (glaube ich) die bevorzugte Art sein, mit diesem Problem in dieser Sprache umzugehen. Es ist jedoch offensichtlich, dass verschiedene Sprachen unterschiedliche Konventionen haben.
Patrick Collins

Antworten:

75

Ja, Ihr Kollege hat Recht: Das ist ein schlechter Code. Wenn ein Fehler lokal behoben werden kann, sollte er sofort behoben werden. Eine Ausnahme sollte nicht ausgelöst und dann sofort behandelt werden.

Dies ist viel sauberer als Ihre Version (die getValueByKey()Methode wurde entfernt):

public String getByKey(String key) {
    if (valuesFromDatabase.containsKey(key)) {
        return valuesFromDatabase.get(key);
    } else {
        String defaultValue = defaultValues.get(key);
        valuesFromDatabase.put(key, defaultvalue);
        return defaultValue;
    }
}

Eine Ausnahme sollte nur ausgelöst werden, wenn Sie nicht wissen, wie Sie den Fehler lokal beheben können.

BЈовић
quelle
14
Danke für die Antwort. Ich bin dieser Kollege (mir hat der Originalcode nicht gefallen), ich habe die Frage nur neutral formuliert, damit sie nicht geladen wird :)
Konrad Morawski
59
Erstes Anzeichen von Wahnsinn: Du beginnst mit dir selbst zu reden.
Robert Harvey
1
@ BЈовић: Nun, in diesem Fall denke ich, dass es in Ordnung ist, aber was ist, wenn Sie zwei Varianten benötigen - eine Methode zum Abrufen des Werts ohne automatische Erstellung fehlender Schlüssel und eine mit? Was ist Ihrer Meinung nach die sauberste (und trockenste) Alternative?
Doc Brown
3
@RobertHarvey :) Jemand anderes hat diesen Code geschrieben, eine separate Person. Ich war der Rezensent
Konrad Morawski
1
@Alvaro, Ausnahmen häufig zu verwenden, ist kein Problem. Ungeachtet der Sprache ist es ein Problem, Ausnahmen schlecht zu verwenden.
kdbanman
11

Ich würde diese Verwendung von Ausnahmen nicht als Antimuster bezeichnen, nur nicht die beste Lösung für das Problem, ein komplexes Ergebnis zu kommunizieren.

Die beste Lösung (vorausgesetzt, Sie arbeiten noch mit Java 7) ist die Verwendung von Guava's Optional. Ich bin nicht einverstanden, dass es in diesem Fall hackisch wäre. Ausgehend von Guavas ausführlicher Erklärung von Optional scheint mir dies ein perfektes Beispiel für die Verwendung zu sein. Sie unterscheiden zwischen "no value found" und "value of null found".

Mike Partridge
quelle
1
Ich glaube nicht, dass Optionales null halten kann. Optional.of()Nimmt nur eine Nicht-Null-Referenz und Optional.fromNullable()behandelt Null als "Wert nicht vorhanden".
Navin,
1
@ Navin es kann nicht null halten, aber Sie haben Optional.absent()zu Ihrer Verfügung. Und so Optional.fromNullable(string)wird gleich sein, Optional.<String>absent()wenn stringnull war, oder Optional.of(string)wenn es nicht war.
Konrad Morawski
@ KonradMorawski Ich dachte, das Problem im OP war, dass man nicht zwischen einer Nullzeichenfolge und einer nicht vorhandenen Zeichenfolge unterscheiden konnte, ohne eine Ausnahme auszulösen. Optional.absent()deckt eines dieser Szenarien ab. Wie stellst du den anderen dar?
Navin,
@ Navin mit einem tatsächlichen null.
Konrad Morawski
4
@KonradMorawski: Das klingt nach einer wirklich schlechten Idee. Ich kann mir kaum einen klareren Weg vorstellen, um zu sagen: "Diese Methode kehrt niemals zurück null!" als es als zurück zu erklären Optional<...>. . .
Ruakh
8

Da es keine Überlegungen zur Leistung gibt und es sich um ein Implementierungsdetail handelt, spielt es letztendlich keine Rolle, für welche Lösung Sie sich entscheiden. Aber ich muss zustimmen, dass es ein schlechter Stil ist. wobei der Schlüssel fehlt etwas , dass Sie wissen , wird passieren, und Sie es sogar mehr als einen Anruf auf dem Stapel nicht umgehen, das ist , wo Ausnahmen am nützlichsten sind.

Der Tupel-Ansatz ist ein bisschen hacky, weil das zweite Feld bedeutungslos ist, wenn der Boolesche Wert falsch ist. Zuvor zu prüfen, ob der Schlüssel vorhanden ist, ist albern, da die Karte den Schlüssel zweimal nachschlägt. (Nun, das machst du schon, also in diesem Fall dreimal.) Die OptionalLösung ist perfekt für das Problem. Es mag ein bisschen ironisch erscheinen, ein nullin einem zu speichern Optional, aber wenn der Benutzer das möchte, kann man nicht nein sagen.

Wie von Mike in den Kommentaren bemerkt, gibt es ein Problem damit; Weder Guava noch Java 8 Optionalerlauben das Speichern von nulls. Daher müssten Sie Ihre eigenen Rollen erstellen, die zwar einfach sind, aber eine Menge Boilerplate enthalten. Dies kann zu einem Overkill für etwas führen, das nur einmal intern verwendet wird. Sie können auch den Kartentyp auf ändern, die Map<String, Optional<String>>Handhabung von Optional<Optional<String>>s ist jedoch umständlich.

Ein vernünftiger Kompromiss könnte darin bestehen, die Ausnahme beizubehalten, aber ihre Rolle als "alternative Rückkehr" anzuerkennen. Erstellen Sie eine neue geprüfte Ausnahme mit deaktiviertem Stack-Trace und werfen Sie eine vorab erstellte Instanz davon, die Sie in einer statischen Variablen behalten können. Dies ist billig - möglicherweise so günstig wie eine lokale Niederlassung - und Sie können nicht vergessen, damit umzugehen, sodass das Fehlen eines Stack-Trace kein Problem darstellt.

Doval
quelle
1
Nun, in Wirklichkeit ist es nur Map<String, String>auf der Ebene der Datenbanktabelle, da die valuesSpalte von einem Typ sein muss. Es gibt jedoch eine Wrapper-DAO-Ebene, die jedes Schlüssel-Wert-Paar stark typisiert. Aber ich habe dies aus Gründen der Klarheit weggelassen. (Natürlich könnten Sie stattdessen eine einzeilige Tabelle mit n Spalten haben, aber das Hinzufügen jedes neuen Werts erfordert die Aktualisierung des Tabellenschemas, sodass das Entfernen der Komplexität, die mit dem Parsen verbunden ist, zu Lasten der Einführung einer anderen Komplexität an anderer Stelle geht.)
Konrad Morawski
Guter Punkt zum Tupel. In der Tat, was (false, "foo")soll das heißen?
Konrad Morawski,
Zumindest bei Guava's Optional führt der Versuch, eine Null in eine Ausnahme zu setzen, zu einer Ausnahme. Sie müssten Optional.absent () verwenden.
Mike Partridge
@ MikePartridge Guter Punkt. Ein hässlicher Workaround wäre, die Karte auf zu ändern, Map<String, Optional<String>>und dann würden Sie mit enden Optional<Optional<String>>. Eine weniger verwirrende Lösung wäre, ein eigenes zulässiges optionales Element nulloder einen ähnlichen algebraischen Datentyp zu erstellen, der nicht zulässig nullist , jedoch drei Zustände aufweist - nicht vorhanden, null oder vorhanden. Beide sind einfach zu implementieren, obwohl die Menge der betroffenen Boilerplate für etwas hoch ist, das nur intern verwendet wird.
Doval
2
"Da es keine Überlegungen zur Leistung gibt und es sich um ein Implementierungsdetail handelt, spielt es letztendlich keine Rolle, für welche Lösung Sie sich entscheiden." "ist für CLR - Ausnahmen aktiviert.
Stephen
5

Es gibt diese Preferences-Klasse, die ein Bucket für Schlüssel-Wert-Paare ist. Nullwerte sind legal (das ist wichtig). Wir gehen davon aus, dass bestimmte Werte möglicherweise noch nicht gespeichert wurden, und möchten diese Fälle automatisch behandeln, indem wir sie auf Anforderung mit einem vordefinierten Standardwert initialisieren.

Das Problem ist genau das. Aber Sie haben die Lösung bereits selbst gepostet:

Eine Variation dieses Ansatzes könnte Guavas Optional (Guava wird bereits im gesamten Projekt verwendet) anstelle eines benutzerdefinierten Tupels verwenden, und dann können wir zwischen Optional.absent () und "richtigen" Null unterscheiden . Es fühlt sich jedoch aus Gründen, die leicht zu erkennen sind, immer noch hackisch an - die Einführung von zwei Ebenen der "Nullheit" scheint das Konzept zu missbrauchen, das ursprünglich für die Schaffung von Optionals verantwortlich war.

Verwenden Sie jedoch nicht null oder Optional . Sie können und sollten Optionalnur verwenden. Für Ihr "hackisches" Gefühl verwenden Sie es einfach verschachtelt, so dass Sie am Ende Optional<Optional<String>>explizit erkennen, dass sich möglicherweise ein Schlüssel in der Datenbank befindet (erste Optionsebene) und dass er einen vordefinierten Wert enthält (zweite Optionsebene).

Dieser Ansatz ist besser als die Verwendung von Ausnahmen und leicht verständlich, solange er Optionalfür Sie nicht neu ist.

Beachten Sie auch, dass Optionaleinige Komfortfunktionen vorhanden sind, sodass Sie nicht die ganze Arbeit selbst erledigen müssen. Diese schließen ein:

  • static static <T> Optional<T> fromNullable(T nullableReference)um Ihre Datenbankeingaben in die OptionalTypen umzuwandeln
  • abstract T or(T defaultValue) um den Schlüsselwert der inneren Optionsebene abzurufen oder (falls nicht vorhanden) Ihren Standardschlüsselwert abzurufen
valenterry
quelle
1
Optional<Optional<String>>sieht böse aus, obwohl es etwas Charme hat, muss ich zugeben
:)
Können Sie weiter erklären, warum es böse aussieht? Es ist eigentlich das gleiche Muster wie List<List<String>>. Sie können natürlich (sofern die Sprache dies zulässt) einen neuen Typ DBConfigKeyerstellen, der den Zeilenumbruch umschließt Optional<Optional<String>>und ihn besser lesbar macht, wenn Sie dies vorziehen.
Valentinstag,
2
@valenterry Es ist ganz anders. Eine Liste von Listen ist eine legitime Möglichkeit, bestimmte Arten von Daten zu speichern. Ein optionales oder optionales Element ist eine atypische Methode, um zwei Arten von Problemen anzuzeigen, die die Daten haben könnten.
Ypnypn
Eine Option einer Option ähnelt einer Liste von Listen, in denen jede Liste ein oder keine Elemente enthält. Es ist im Wesentlichen das gleiche. Nur weil es in Java nicht oft verwendet wird, heißt das noch lange nicht, dass es von Natur aus schlecht ist.
Valentinstag
1
@valenterry böse in dem Sinne, dass Jon Skeet bedeutet; also nicht ganz schlecht, aber ein bisschen böser "cleverer Code". Ich denke, es ist nur etwas barock, optional oder optional. Es ist nicht explizit klar, welcher Grad an Optionalität für was steht. Ich hätte eine doppelte Einstellung, wenn ich im Code von jemandem darauf stoßen würde. Sie könnten Recht haben, dass es sich seltsam anfühlt, nur weil es ungewöhnlich und unidiomatisch ist. Vielleicht ist es nichts Besonderes für einen funktionalen Sprachprogrammierer mit seinen Monaden und allem. Ich würde es zwar nicht selbst versuchen, aber ich würde Ihre Antwort trotzdem +1 geben, weil es interessant ist
Konrad Morawski,
5

Ich weiß, dass ich zu spät zur Party komme, aber auf jeden Fall ähnelt Ihr Anwendungsfall dem, in dem Java auchProperties eine Reihe von Standardeigenschaften definiert, die überprüft werden, wenn von der Instanz kein entsprechender Schlüssel geladen wird.

Sehen Sie sich an, wie die Implementierung durchgeführt wird Properties.getProperty(String)(ab Java 7):

Object oval = super.get(key);
String sval = (oval instanceof String) ? (String)oval : null;
return ((sval == null) && (defaults != null)) ? defaults.getProperty(key) : sval;

Wie Sie zitiert haben, ist es nicht unbedingt erforderlich, "Ausnahmen zu missbrauchen, um den Datenfluss zu steuern".

Ein ähnlicher, aber etwas knapperer Ansatz für die Antwort von @ BЈовић kann auch lauten:

public String getByKey(String key) {
    if (!valuesFromDatabase.containsKey(key)) {
        valuesFromDatabase.put(key, defaultValues.get(key));
    }
    return valuesFromDatabase.get(key);
}
hjk
quelle
4

Obwohl ich denke, dass die Antwort von @ Bћовић in Ordnung ist, falls getValueByKeyes nirgendwo anders gebraucht wird, denke ich nicht, dass Ihre Lösung schlecht ist, falls Ihr Programm beide Anwendungsfälle enthält:

  • Abruf per Schlüssel mit automatischer Erstellung, falls der Schlüssel vorher nicht existiert, und

  • Abrufen ohne diesen Automatismus, ohne irgendetwas in der Datenbank, dem Repository oder der Schlüsselkarte zu getValueByKeyändern (denken Sie daran, öffentlich zu sein, nicht privat)

Wenn dies Ihre Situation ist und die Leistung akzeptabel ist, denke ich, dass Ihre vorgeschlagene Lösung völlig in Ordnung ist. Es hat den Vorteil, dass Duplikationen des Abrufcodes vermieden werden, es ist nicht auf Frameworks von Drittanbietern angewiesen und es ist ziemlich einfach (zumindest in meinen Augen).

Tatsächlich hängt es in einer solchen Situation vom Kontext ab, ob ein fehlender Schlüssel eine "außergewöhnliche" Situation ist oder nicht. Für einen Kontext, in dem es ist, wird so etwas getValueByKeybenötigt. In einem Kontext, in dem eine automatische Schlüsselerstellung erwartet wird, ist es durchaus sinnvoll, eine Methode bereitzustellen, die die bereits verfügbare Funktion wiederverwendet, die Ausnahme verschluckt und ein anderes Fehlerverhalten bietet. Dies kann als Erweiterung oder "Dekorator" interpretiert werden getValueByKey, weniger als eine Funktion, bei der die "Ausnahme für den Kontrollfluss missbraucht wird".

Natürlich gibt es eine dritte Alternative: Erstellen Sie eine dritte, private Methode, die die Tuple<Boolean, String>von Ihnen vorgeschlagene zurückgibt, und verwenden Sie diese Methode sowohl in getValueByKeyals auch wieder getByKey. Für einen komplizierteren Fall ist das in der Tat die bessere Alternative, aber für einen so einfachen Fall, wie er hier gezeigt wird, riecht es nach Überentwicklung, und ich bezweifle, dass der Code auf diese Weise wirklich besser gewartet werden kann. Ich bin hier mit der obersten Antwort von Karl Bielefeldt :

msgstr "Sie sollten sich nicht schlecht fühlen, wenn Sie Ausnahmen verwenden, um Ihren Code zu vereinfachen".

Doc Brown
quelle
3

Optionalist die richtige Lösung. Wenn Sie es jedoch vorziehen, gibt es eine Alternative, die sich weniger als "zwei Nullen" anfühlt, betrachten Sie einen Sentinel.

Definieren Sie eine private statische Zeichenfolge keyNotFoundSentinelmit einem Wert new String(""). *

Jetzt kann die private Methode return keyNotFoundSentineleher als throw new KeyNotFoundException(). Die öffentliche Methode kann nach diesem Wert suchen

String rval = getValueByKey(key);
if (rval == keyNotFoundSentinel) { // reference equality, not string.equals
    ... // generate default value
} else {
    return rval;
}

In Wirklichkeit nullhandelt es sich um einen Sonderfall eines Sentinels. Dies ist ein von der Sprache definierter Sentinel-Wert mit einigen besonderen Verhaltensweisen (dh einem genau definierten Verhalten, wenn Sie eine Methode aufrufen null). Es ist einfach so nützlich, einen solchen Sentinel zu haben, wie ihn fast jede Sprache verwendet.

* Wir verwenden Java absichtlich new String("")und nicht nur "", um zu verhindern, dass es den String "interniert". Dies würde ihm den gleichen Verweis geben wie jedem anderen leeren String. Da wir diesen zusätzlichen Schritt ausgeführt haben, ist garantiert, dass es sich bei dem von angegebenen String um keyNotFoundSentineleine eindeutige Instanz handelt, die auf keinen Fall in der Map selbst angezeigt werden darf.

Cort Ammon
quelle
Dies ist meiner Meinung nach die beste Antwort.
fgp
2

Lernen Sie aus dem Framework, das aus allen Schwachpunkten von Java gelernt hat:

.NET bietet zwei weitaus elegantere Lösungen für dieses Problem:

Letzteres ist in Java sehr einfach zu schreiben, ersteres erfordert lediglich eine "starke Referenz" -Hilfeklasse.

Ben Voigt
quelle
Eine kovarianzfreundliche Alternative zum DictionaryMuster wäre T TryGetValue(TKey, out bool).
Supercat