Lancer une exception est-il un anti-modèle ici?

33

Je viens d'avoir une discussion sur un choix de conception après une révision du code. Je me demande quelles sont vos opinions.

Il y a cette Preferencesclasse, qui est un compartiment pour les paires clé-valeur. Les valeurs nulles sont légales (c'est important). Nous nous attendons à ce que certaines valeurs ne soient pas encore enregistrées, et nous souhaitons gérer ces cas automatiquement en les initialisant avec une valeur par défaut prédéfinie à la demande.

La solution discutée utilisait le modèle suivant (NOTE: ce n'est pas le code réel, bien entendu - c'est simplifié à des fins d'illustration):

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();
        }
    }
}

Il a été critiqué comme un anti-modèle - abusant des exceptions pour contrôler le flux . KeyNotFoundException- mis en oeuvre pour ce seul cas d'utilisation - ne sera jamais considéré comme étant hors de la portée de cette classe.

Il s’agit essentiellement de deux méthodes jouant avec chercher simplement pour communiquer quelque chose l’une à l’autre.

Le fait que la clé ne soit pas présente dans la base de données n'est pas alarmant ni exceptionnel. Nous nous attendons à ce que cela se produise chaque fois qu'un nouveau paramètre de préférence est ajouté. D'où le mécanisme qui l'initialise de manière élégante avec une valeur par défaut si nécessaire.

Le contre-argument était que getValueByKey- la méthode privée - telle que définie actuellement n’a aucun moyen naturel d’informer la méthode publique à la fois de la valeur et de la présence de la clé. (Si ce n'est pas le cas, il faut l'ajouter pour que la valeur puisse être mise à jour).

Retourner nullserait ambigu, car nullc'est une valeur parfaitement légale, donc on ne peut pas dire si cela signifiait que la clé n'était pas là, ou s'il y avait un null.

getValueByKeydevrait renvoyer une sorte de Tuple<Boolean, String>, bool mis à true si la clé est déjà là, afin que nous puissions distinguer entre (true, null)et (false, null). (Un outparamètre peut être utilisé en C #, mais c'est Java).

Est-ce une alternative plus agréable? Oui, il faudrait définir une classe à usage unique comme effet Tuple<Boolean, String>, mais ensuite, nous nous en débarrassons KeyNotFoundException, ce genre d'équilibre. Nous évitons également la surcharge liée à la gestion d'une exception, bien que ce ne soit pas important du point de vue pratique: il n'y a aucune considération de performance à prendre en compte, c'est une application cliente et ce n'est pas comme si les préférences des utilisateurs seraient récupérées des millions de fois par seconde.

Une variante de cette approche pourrait utiliser celle de Guava Optional<String>(celle-ci est déjà utilisée dans tout le projet) au lieu d'une coutume Tuple<Boolean, String>, ce qui permet de faire la différence entre Optional.<String>absent()et "correct" null. Il se sent toujours hackish, cependant, pour des raisons faciles à comprendre - l'introduction de deux niveaux de "nullité" semble abuser du concept qui sous-tendait la création de Optionals.

Une autre option serait de vérifier explicitement si la clé existe (ajoutez une boolean containsKey(String key)méthode et appelez getValueByKeyuniquement si nous avons déjà affirmé qu'elle existe).

Enfin, on pourrait aussi utiliser la méthode privée, mais la réalité getByKeyest un peu plus complexe que mon exemple de code.

Je vais peut-être couper les cheveux en quatre, mais je suis curieux de savoir sur quoi vous pariez être le plus proche des meilleures pratiques dans ce cas. Je n'ai pas trouvé de réponse dans les guides de style d'Oracle ou de Google.

L'utilisation d'exceptions comme dans l'exemple de code est-elle un anti-modèle ou est-elle acceptable étant donné que les alternatives ne sont pas très propres, non plus? Si c'est le cas, dans quelles circonstances cela irait-il? Et vice versa?

Konrad Morawski
la source
1
@ GlenH7, la réponse acceptée (et la plus votée) résume "vous devez les utiliser [exceptions] dans les cas où ils facilitent la gestion des erreurs avec moins d'encombrement du code". Je suppose que je demande ici si les alternatives que j'ai énumérées devraient être considérées comme "moins d'encombrement de code".
Konrad Morawski
1
J'ai rétracté mon VTC - vous demandez quelque chose de similaire mais différent du duplicata que j'ai suggéré.
3
Je pense que la question devient plus intéressante quand elle getValueByKeyest aussi publique.
Doc Brown
2
Pour référence future, vous avez fait la bonne chose. Cela aurait été hors sujet dans la révision du code car c'est un exemple de code.
RubberDuck
1
Juste pour ajouter --- c'est un Python parfaitement idiomatique, et serait (je pense) le moyen préféré pour traiter ce problème dans cette langue. Évidemment, différentes langues ont des conventions différentes.
Patrick Collins

Réponses:

75

Oui, votre collègue a raison: c’est un mauvais code. Si une erreur peut être traitée localement, elle doit être traitée immédiatement. Une exception ne doit pas être levée puis traitée immédiatement.

Ceci est beaucoup plus propre que votre version (la getValueByKey()méthode est supprimée):

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

Une exception ne doit être levée que si vous ne savez pas comment résoudre l'erreur localement.

BЈовић
la source
14
Merci d'avoir répondu. Pour mémoire, je suis ce collègue (je n'ai pas aimé le code original), je viens de formuler la question de manière neutre afin qu'elle ne soit pas chargée :)
Konrad Morawski
59
Premier signe de folie: vous commencez à vous parler.
Robert Harvey
1
@ BЈовић: Eh bien, dans ce cas, je pense que c'est correct, mais si vous avez besoin de deux variantes - une méthode permettant de récupérer la valeur sans création automatique des clés manquantes, et une avec? Selon vous, quelle est l'alternative la plus propre (et sèche)?
Doc Brown
3
@RobertHarvey :) Quelqu'un d'autre est l'auteur de cette partie de code, une personne bien à part, dont j'étais le critique
Konrad Morawski
1
@ Alfaro, l'utilisation fréquente d'exceptions n'est pas un problème. L'utilisation incorrecte des exceptions est un problème, quelle que soit la langue.
kdbanman
11

Je n'appellerais pas cette utilisation des exceptions un anti-motif, mais ce n'est pas la meilleure solution au problème de la communication d'un résultat complexe.

La meilleure solution (en supposant que vous soyez toujours sur Java 7) serait d'utiliser Guava's Optional; Je ne suis pas d'accord que son utilisation dans ce cas serait hack. Il me semble, à la lumière de l’explication détaillée donnée par Guava sur Option , qu’il s’agit d’un parfait exemple de son utilisation. Vous faites la distinction entre "aucune valeur trouvée" et "valeur de null trouvée".

Mike Partridge
la source
1
Je ne pense pas Optionalpouvoir tenir nul. Optional.of()prend seulement une référence non-nulle et Optional.fromNullable()traite null comme "valeur non présente".
Navin
1
@Navin il ne peut pas tenir à zéro, mais vous avez Optional.absent()à votre disposition. Et ainsi, Optional.fromNullable(string)sera égal à Optional.<String>absent()si stringétait null, ou Optional.of(string)si ce n'était pas.
Konrad Morawski
@ KonradMorawski Je pensais que le problème dans l'OP était qu'il était impossible de faire la distinction entre une chaîne nulle et une chaîne non existante sans lever une exception. Optional.absent()couvre l'un de ces scénarios. Comment représentez-vous l'autre?
Navin
@Navin avec un réel null.
Konrad Morawski
4
@ KonradMorawski: Cela semble être une très mauvaise idée. Je peux difficilement penser à une façon plus claire de dire "cette méthode ne revient jamais null!" que de le déclarer comme revenant Optional<...>. . .
Ruakh
8

Puisqu'il n'y a pas de considérations de performances et qu'il s'agit d'un détail d'implémentation, la solution que vous choisissez n'a finalement pas d'importance. Mais je dois convenir que c'est un mauvais style; la clé d'absence est quelque chose que vous savez qui va arriver, et vous ne la gérez même pas plus d'un appel dans la pile, c'est là que les exceptions sont les plus utiles.

L'approche en tuple est un peu hacky car le second champ n'a pas de sens lorsque le booléen est faux. Vérifier si la clé existe au préalable est idiot, car la carte la recherche deux fois. (Eh bien, vous le faites déjà, alors dans ce cas-ci trois fois.) La Optionalsolution convient parfaitement au problème. Il peut sembler un peu ironique de stocker un nulldans unOptional , mais si c'est ce que l'utilisateur veut faire, vous ne pouvez pas dire non.

Comme Mike l'a noté dans ses commentaires, cela pose un problème. ni Guava ni Java 8 ne Optionalpermettent de stocker des nullfichiers. Par conséquent, vous auriez besoin de lancer le vôtre, ce qui - bien que simple - implique une bonne quantité de passe-partout, donc peut-être excessif pour quelque chose qui ne sera utilisé qu'une fois en interne. Vous pouvez également changer le type de la carte en Map<String, Optional<String>>, mais la manipulation Optional<Optional<String>>devient difficile.

Un compromis raisonnable pourrait consister à conserver l'exception, mais à reconnaître son rôle en tant que "retour alternatif". Créez une nouvelle exception vérifiée avec la trace de pile désactivée et jetez-en une instance pré-créée que vous pouvez conserver dans une variable statique. Cela n’est pas cher - peut - être aussi bon marché qu’une agence locale - et vous ne pouvez pas oublier de le gérer, l’absence de trace de pile n’est donc pas un problème.

Doval
la source
1
En réalité, il s’agit uniquement Map<String, String>de la table de la base de données, car la valuescolonne doit être d’un seul type. Il existe cependant une couche DAO d'encapsuleur qui définit en force chaque paire clé-valeur. Mais j'ai omis ceci pour plus de clarté. (Bien entendu, vous pouvez utiliser une table à une ligne avec n colonnes, mais l'ajout de chaque nouvelle valeur nécessite la mise à jour du schéma de la table. La suppression de la complexité associée à la nécessité de les analyser revient toutefois à introduire une autre complexité ailleurs).
Konrad Morawski
Bon point à propos du tuple. En effet, que serait (false, "foo")censé vouloir dire?
Konrad Morawski
Au moins avec Guava's Optional, essayer de mettre une valeur nulle dans les résultats d'une exception. Vous devez utiliser Optional.absent ().
Mike Partridge
@ MikePartridge Bon point. Une solution de contournement désagréable consisterait à modifier la carte Map<String, Optional<String>>et à vous retrouver avec Optional<Optional<String>>. Une solution moins déroutante serait de lancer votre propre option qui autorise null, ou un type de données algébrique similaire qui interdit nullmais a 3 états - absent, nul ou présent. Les deux sont simples à mettre en œuvre, bien que la quantité de passe-partout impliqué soit élevée pour quelque chose qui ne sera utilisé qu'en interne.
Doval
2
"Comme il n'y a pas de considérations de performances et qu'il s'agit d'un détail d'implémentation, la solution choisie importe peu" - Sauf que si vous utilisiez C #, une exception ennuyera quiconque tente de déboguer avec "Break quand une exception est levée. "est activé pour les exceptions CLR.
Stephen
5

Il y a cette classe Preferences, qui est un compartiment pour les paires clé-valeur. Les valeurs nulles sont légales (c'est important). Nous nous attendons à ce que certaines valeurs ne soient pas encore enregistrées, et nous souhaitons gérer ces cas automatiquement en les initialisant avec une valeur par défaut prédéfinie à la demande.

Le problème est exactement ceci. Mais vous avez déjà posté la solution vous-même:

Une variante de cette approche pourrait utiliser une option de Guava (Guava est déjà utilisé dans tout le projet) au lieu d'un tuple personnalisé, ce qui permet de différencier Facultatif.absent () de null "approprié" . Il se sent toujours hackish, cependant, pour des raisons faciles à comprendre - l'introduction de deux niveaux de "nullité" semble abuser du concept qui sous-tendait la création d'options à la première place.

Cependant, n'utilisez pas null ou Optional . Vous pouvez et devriez utiliser Optionalseulement. Pour votre sentiment "hack", utilisez-le simplement imbriqué, ce Optional<Optional<String>>qui indique explicitement qu'il peut y avoir une clé dans la base de données (première couche d'options) et qu'elle peut contenir une valeur prédéfinie (deuxième couche d'options).

Cette approche vaut mieux que d’utiliser des exceptions et elle est assez facile à comprendre tant qu’elle Optionaln’est pas nouvelle pour vous.

Veuillez également noter que Optionalcertaines fonctions de confort vous évitent d'avoir à effectuer tout le travail vous-même. Ceux-ci inclus:

  • static static <T> Optional<T> fromNullable(T nullableReference)convertir votre entrée de base de données en Optionaltypes
  • abstract T or(T defaultValue) pour obtenir la valeur de clé de la couche d'option interne ou (si non présent) obtenir la valeur de clé par défaut
valenterry
la source
1
Optional<Optional<String>>ça a l'air mauvais, même si ça a du charme, je dois l'avouer
:)
Pouvez-vous expliquer davantage pourquoi cela semble mauvais? C'est en fait le même motif que List<List<String>>. Vous pouvez bien sûr (si le langage le permet) créer un nouveau type, comme celui-ci, DBConfigKeyqui le Optional<Optional<String>>rend plus lisible si vous le préférez.
valenterry
2
@valenterry C'est très différent. Une liste de listes est un moyen légitime de stocker certains types de données. une option d'une option est une manière atypique d'indiquer deux types de problèmes que les données pourraient avoir.
Ypnypn
Une option d'une option ressemble à une liste de listes où chaque liste ne contient qu'un ou aucun élément. C'est essentiellement la même chose. Le fait qu’il ne soit pas souvent utilisé en Java ne signifie pas qu’il est intrinsèquement mauvais.
valenterry
1
@valenterry evil en ce sens que Jon Skeet signifie; donc pas vraiment mauvais, mais un peu méchant, "code intelligent". Je suppose que c'est juste un peu baroque, une option d'une option. On ne sait pas clairement quel niveau de faculté représente quoi. J'aurais une double prise si je le rencontrais dans le code de quelqu'un. Vous avez peut-être raison de dire que ça fait bizarre, juste parce que c'est inhabituel, unidiomatique. Peut-être n’est-ce pas fantastique pour un programmeur de langage fonctionnel, avec ses monades et tout. Même si je ne voudrais pas y aller moi-même, j'ai quand même +1 votre réponse, car elle est intéressante
Konrad Morawski
5

Je sais que je suis en retard pour la fête, mais de toute façon votre cas d'utilisation ressemble à celui de Java quiProperties définit également un ensemble de propriétés par défaut, qui sera vérifié si aucune clé correspondante n'est chargée par l'instance.

Voici comment se déroule la mise en œuvre Properties.getProperty(String)(à partir de Java 7):

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

Il n’est vraiment pas nécessaire de "abuser des exceptions pour contrôler le flux", comme vous l’avez mentionné.

Une approche similaire, mais légèrement plus concise, de la réponse de @BЈовић peut également être:

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

Bien que je pense que la réponse de @Bоовић soit correcte au cas où vous n'auriez getValueByKeybesoin de rien d'autre, je ne pense pas que votre solution soit mauvaise si votre programme contient les deux cas d'utilisation:

  • récupération par clé avec création automatique au cas où la clé n'existerait pas auparavant, et

  • récupération sans cet automatisme, sans rien changer dans la base de données, le référentiel ou la carte de clé (pensez à getValueByKeyêtre public, pas privé)

Si tel est votre cas, et tant que la performance est acceptable, je pense que la solution proposée est tout à fait acceptable. Il a l'avantage d'éviter la duplication du code de récupération, il ne s'appuie pas sur des frameworks tiers, et c'est assez simple (du moins à mes yeux).

En fait, dans une telle situation, cela dépend du contexte si une clé manquante est une situation "exceptionnelle" ou non. Pour un contexte où il est, quelque chose comme getValueByKeyest nécessaire. Dans un contexte où la création automatique de clé est attendue, fournir une méthode qui réutilise la fonction déjà disponible, avalait l'exception et fournissait un comportement de défaillance différent, est parfaitement logique. Cela peut être interprété comme une extension ou un "décorateur" de getValueByKey, pas tellement comme une fonction où "l'exception est utilisée abusivement pour un écoulement de contrôle".

Bien sûr, il existe une troisième alternative: créez une troisième méthode, privée, renvoyant le Tuple<Boolean, String>, comme vous l'avez suggéré, et réutilisez cette méthode dans getValueByKeyet getByKey. Pour un cas plus compliqué, c'est peut-être la meilleure solution, mais pour un cas aussi simple que celui présenté ci-dessus, cela a une odeur d'ingénierie à mon humble avis, et je doute que le code devienne vraiment plus facile à gérer de cette façon. Je suis ici avec la plus haute réponse ici par Karl Bielefeldt :

msgstr "" "vous ne devriez pas vous sentir mal à l 'aide des exceptions quand cela simplifie votre code".

Doc Brown
la source
3

Optionalest la bonne solution. Cependant, si vous préférez, il existe une alternative qui a moins de "deux nuls", considérez une sentinelle.

Définir une chaîne statique privée keyNotFoundSentinel, avec une valeur new String(""). *

Maintenant, la méthode privée peut return keyNotFoundSentinelplutôt que throw new KeyNotFoundException(). La méthode publique peut vérifier cette valeur

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

En réalité, nullc'est un cas particulier d'une sentinelle. C'est une valeur sentinelle définie par le langage, avec quelques comportements spéciaux (un comportement bien défini si vous appelez une méthode null). Il se trouve que c'est tellement utile d'avoir une sentinelle comme celle que presque toutes les langues utilisent.

* Nous utilisons intentionnellement new String("")plutôt que simplement ""pour empêcher Java "d'interner" la chaîne, ce qui lui donnerait la même référence que toute autre chaîne vide. En raison de cette étape supplémentaire, nous avons la garantie que la chaîne référencée par keyNotFoundSentinelest une instance unique, ce dont nous avons besoin pour nous assurer qu’elle ne pourra jamais apparaître dans la carte elle-même.

Cort Ammon - Rétablir Monica
la source
Ceci, à mon humble avis, est la meilleure réponse.
fgp
2

Apprenez du cadre qui a appris de tous les problèmes de Java:

.NET fournit deux solutions bien plus élégantes à ce problème, illustrées par:

Ce dernier est très facile à écrire en Java, le premier nécessite simplement une classe d’aide "référence forte".

Ben Voigt
la source
Une alternative au Dictionarymodèle qui conviendrait aux covariances serait T TryGetValue(TKey, out bool).
Supercat