Le service doit-il lever une exception ou revenir si aucun élément n'est spécifié pour la suppression

12

J'ai un morceau de code qui peut être représenté comme:

public class ItemService {

    public void DeleteItems(IEnumerable<Item> items)
    {
        // Save us from possible NullReferenceException below.
        if(items == null)
            return;

        foreach(var item in items)
        {
            // For the purpose of this example, lets say I have to iterate over them.
            // Go to database and delete them.
        }
    }
}

Maintenant, je me demande si c'est la bonne approche ou dois-je lever l'exception. Je peux éviter les exceptions, car retourner serait la même chose que d'itérer sur une collection vide, ce qui signifie qu'aucun code important n'est exécuté de toute façon, mais d'un autre côté, je cache peut-être des problèmes quelque part dans le code, car pourquoi quelqu'un voudrait-il appeler DeleteItemsavec nullparamètre? Cela peut indiquer qu'il y a un problème ailleurs dans le code.

C'est un problème que j'ai généralement avec les méthodes dans les services, car la plupart d'entre elles font quelque chose et ne renvoient pas de résultat, donc si quelqu'un transmet des informations invalides, il n'y a rien à faire pour le service, donc il revient.

FCin
la source
2
C'est juste mon opinion personnelle, mais j'ai toujours trouvé qu'il était plus logique qu'une méthode lève une exception lorsqu'elle fait quelque chose d'inattendu (exceptionnel). Dans votre cas, je lèverais une exception InvalidOperationException si quelqu'un essayait de supprimer des éléments null / 0.
Falgantil
1
@gnat Ma question concerne spécifiquement les services, en raison de leur utilisation et de leur finalité. Ce «doublon» ne parle que des cas généraux et la réponse principale se contredit même dans le cadre de ma méthode exacte.
FCin
2
Est-il possible que l'énumérable soit vide au lieu de null? Souhaitez-vous gérer cela différemment?
JAD
13
@Falgantil Je peux voir null être digne d'une exception, mais je pense qu'il est absurde de jeter une exception sur une liste vide.
Kevin

Réponses:

58

Ce sont deux questions différentes.

Devriez-vous accepter null? Cela dépend de votre politique générale nulldans la base de code. À mon avis, interdire nullpartout sauf là où cela est explicitement documenté est une très bonne pratique, mais c'est encore mieux de s'en tenir à la convention que votre base de code a déjà.

Devriez-vous accepter la collection vide? À mon avis: OUI, absolument. Il faut beaucoup plus d'efforts pour restreindre tous les appelants à des collections non vides que pour faire ce qui est mathématiquement juste - même si cela surprend certains développeurs qui ne savent pas exactement le concept de zéro.

Kilian Foth
la source
9
Dans tous les cas, si vous allez lancer, alors il n'y a pas beaucoup d'utilité à lancer AhaINoticedYouPassingInNullExceptionlorsque le runtime vous offre déjà la possibilité de lancer NullReferenceExceptionpour vous, sans que vous n'écrivez de code du tout. Il existe un certain utilitaire (par exemple, votre outil de couverture de code vous dira si vous avez ou non un test unitaire pour passer null dans le premier cas mais pas dans le second), mais aucune exception ne devrait être essayée / interceptée à chaque appel au service, car c'est généralement une erreur de programmeur.
Steve Jessop
2
... vous ne devriez être immédiatement en train d'attraper des exceptions levées en raison de paramètres sommaires, si vous savez que ce que vous transmettez est nul dans certains cas attendus, et que vous souhaitez activement que la fonction que vous appelez le valide pour vous. Comme Kilian le dit dans la réponse, votre politique générale pourrait être d'interdire null partout où cela n'est pas explicitement autorisé. Ensuite, vous n'attraperiez NullReferenceExceptionni AhaINoticedYouPassingInNullExceptionn'importe où, sauf dans un code de récupération après sinistre de haut niveau. Et ce gestionnaire d'exceptions devrait probablement créer un rapport de bogue :-)
Steve Jessop
2
@FCin: la conception de votre interface doit correspondre à ce dont les utilisateurs ont réellement besoin pour sortir du service. Donc, si chaque appelant veut mettre try / catch autour de cela, alors cette méthode ou une méthode pratique à côté devrait vérifier et ignorer null, car c'est ce que votre public exige. Cependant, comme le dit Kilian, il est généralement préférable de traiter la propagation des valeurs nulles comme une erreur de programmation et de ne pas essayer de continuer sur chaque site d'appel. D'ailleurs, que se passe-t-il si le service sur lequel cette méthode est appelée est null- les contrôleurs contiennent-ils un passe-partout à attraper et à continuer dans ce cas également?
Steve Jessop
2
... dans l'affirmative, il semble que la base de code traite systématiquement les composants manquants comme une condition attendue que vous devez gérer à un niveau bas et continuer. Vous pouvez raisonnablement vous demander "à qui incombe la responsabilité de fournir un service et un énumérable afin que cette opération puisse se poursuivre?". Si la réponse est "quelqu'un", votre fonction vérifie les conditions préalables et le résultat d'une condition préalable échouée serait normalement une exception interceptée à un niveau élevé, pas à chaque appel. Si les éléments requis pour l'opération sont vraiment facultatifs, votre fonction gère un cas d'utilisation attendu .
Steve Jessop
2
ArgumentNullExceptionLancer explicitement un est supérieur à permettre au code intérieur de lancer un NullReferenceException- en regardant simplement le message d'exception, il est évident qu'il s'agit d'une erreur d'entrée plutôt que d'une erreur logique dans le corps de la méthode sur le site de lancement, et garantir une sortie précoce signifie que vous êtes plus susceptible d'avoir laissé d'autres données dans un état valide plutôt que partiellement muté à partir d'un null inattendu ultérieurement.
Miral
9

Valeur nulle

Comme l'a déjà dit @KilianFoth, respectez votre politique générale. Si cela doit être considéré nullcomme un "raccourci" pour une liste vide, faites-le de cette façon.

Si vous n'avez pas de politique cohérente sur les nullvaleurs, je recommanderais la suivante:

nulldevrait être réservé pour représenter des situations qui ne peuvent pas être exprimées par le type "normal", par exemple en utilisant nullpour représenter "je ne sais pas". Et c'est un bon choix, car tous ceux qui essaient d'utiliser cette valeur sans précaution recevront une exception, ce qui est la bonne chose.

L'utilisation nullcomme raccourci pour une liste vide ne se qualifie pas de cette façon, car il existe déjà une représentation parfaite, étant une liste avec zéro élément. Et c'est un mauvais choix technique, car il oblige chaque partie de votre code traitant des listes à vérifier la sténographie valide null.

Liste vide

Pour une DeleteItems()méthode, passer une liste vide signifie effectivement ne rien faire. J'autoriserais cela comme argument, sans lever d'exception, mais simplement revenir rapidement.

Bien sûr, l'appelant pourrait d'abord vérifier l'absence d'éléments et ignorer l' DeleteItems()appel dans ce cas. Si nous parlons d'une API Web, pour des raisons d'efficacité, l'appelant devrait en fait le faire pour éviter le trafic inutile et les latences aller-retour. Mais je ne pense pas que votre API devrait appliquer cela.

Ralf Kleberhoff
la source
1

Lancez une exception et gérez les valeurs NULL dans le code appelant.

En règle générale, essayez d'éviter null en tant que valeurs de paramètre. Cela réduira les NullPointerExceptions en général, car les valeurs nulles seront vraiment une exception.

En plus de cela, regardez le reste de votre code. S'il s'agit d'un modèle courant dans votre projet, restez cohérent.

serprime
la source
Je suis en train de "façonner" la façon dont mes services sont structurés, donc une refactorisation pourrait être nécessaire pour utiliser des entrées non valides, mais pas beaucoup. Mais je suis d'accord avec votre point de vue que les null deviendront une exception une fois que je commencerai à lancer au lieu de les cacher.
FCin
0

De manière générale, la levée d'exceptions devrait être réservée à des situations exceptionnelles, c'est-à-dire si le code n'a pas de ligne de conduite raisonnable à prendre dans le contexte actuel.

Vous pouvez appliquer ce processus de réflexion à cette situation. Étant donné qu'il s'agit d'une classe publique avec une méthode publique, vous avez une API publique et en théorie aucun contrôle sur ce qui lui est transmis.

Votre code n'a aucun contexte sur ce qui l'appelle (comme il ne devrait pas), et il n'y a rien que vous puissiez raisonnablement faire avec une valeur nulle. Ce serait certainement un candidat pour un ArgumentNullException.

richzilla
la source
"si le code n'a aucun plan d'action raisonnable à prendre dans le contexte actuel". Mais ma pensée est, il a une ligne de conduite raisonnable, qui est de retourner nul. Il fait exactement la même chose que lors du passage d'une collection vide, donc si j'autorise la collection vide, alors pourquoi ne le permettrais-je pas null?
FCin
1
@FCin En raison d'une collection vide, vous savez qu'elle est vide. Avec nullvous, vous n'avez pas de collection. Si le code appelant est supposé / devrait fournir une collection à la méthode, il y a quelque chose qui ne va pas, vous devriez donc lancer. La seule raison de traiter un null de la même manière qu'une collection vide est si vous pouvez raisonnablement vous attendre à ce que le code appelant produise des collections null. Comme d'autres réponses l'ont noté, la plupart du temps, quelque chose nullest une anomalie. Si vous les traitez de la même manière que les collections vides, vous ignorez potentiellement un problème ailleurs dans le code.
JAD
@FCin: aussi, si vous nullparlez de vide, cela dépend du contexte de cette méthode. Quelque part ailleurs dans la même base de code, vous pourriez avoir un paramètre IEnumerableou IContainerqui effectue une sorte de filtrage, nullpourrait signifier «pas de filtre» (tout autoriser) tandis qu'un filtre vide signifie ne rien autoriser. Et dans un autre endroit, cela nullpourrait signifier explicitement "je ne sais pas". Donc, étant donné que nullcela ne signifie pas toujours exactement la même chose partout, c'est une bonne idée de ne pas inventer de sens du tout, à moins que ce sens ne soit nécessaire ou au moins utile à quelqu'un.
Steve Jessop
0

Cette question ne semble pas concerner les exceptions, mais nullêtre un argument valable.

Donc, vous devez d'abord et avant tout décider s'il nulls'agit d'une valeur autorisée pour votre argument de cette méthode. Si c'est le cas, vous n'avez pas besoin d'une exception. Si ce n'est pas le cas, vous avez besoin d'une exception.

Que vous souhaitiez autoriser nullou non est discutable, comme en témoignent de nombreux hits Google à ce sujet. Cela signifie que vous n'obtiendrez pas de réponse claire et que cela dépend quelque peu de l'opinion et de la tradition de l'endroit où vous travaillez.

Un autre point litigieux est de savoir si une fonction de bibliothèque doit être vraiment stricte à ce sujet, ou aussi indulgente que possible, tant qu'elle n'essaie pas de «corriger» des paramètres erronés. Comparez cela au monde des protocoles réseau, du transfert de courrier, etc. (qui sont des contrats d'interface, tout comme les méthodes de programmation). Là, généralement, la politique est que l'expéditeur doit se conformer le plus strictement possible au protocole, tandis que le récepteur doit se mettre en quatre pour pouvoir travailler avec tout ce qui se présente.

Vous devez donc décider: est-ce vraiment le travail d'une méthode de bibliothèque d'appliquer une politique assez large comme la nullgestion? Surtout si votre bibliothèque est également utilisée par d'autres personnes (qui peuvent avoir des politiques différentes).

Je préférerais probablement autoriser les nullvaleurs, définir et documenter leur sémantique (c'est- null = empty arrayà- dire dans ce cas), et ne pas lever d'exception, à moins que 99% de votre autre code similaire (code de style "bibliothèque") ne le fasse dans l'autre sens. 'rond.

AnoE
la source
1
"Est-ce vraiment le travail d'une méthode de bibliothèque d'appliquer une politique assez volumineuse comme la gestion nulle?" - dans cette ligne de pensée, les modes d'affirmation et de débogage, qui vous permettent d' aider à appliquer une politique, plutôt que de la faire réellement, si c'est ce que vous voulez faire. Donc, si vous allez avaler nullsur le principe d'être libéral dans ce que vous acceptez, alors la journalisation ou même le lancement serait utile aux personnes dont l' intention est d'être stricte dans ce qu'elles transmettent, mais qui ont bousillé leur code et leurs tests unitaires dans la mesure où ils réussissent de nulltoute façon.
Steve Jessop
@SteveJessop, je ne sais pas vraiment si vous vous opposez à l'esprit de ma réponse, ou si vous poursuivez dans cette voie. Cela dit, je ne propose pas simplement d'avaler null, mais de déclarer ouvertement dans le contrat de la méthode qu'elle est acceptable (ou non, selon la solution proposée par le PO), puis la question de savoir si lever une exception (ou pas) se résout.
AnoE