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

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?

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.

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".

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.

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

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

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".

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.

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:

• Dictionary<TKey, TValue>.TryGetValue(TKey, out TValue)
• Nullable<T>.GetValueOrDefault(T default)

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