Devrais-je accepter des collections vides dans mes méthodes qui les parcourent?

40

J'ai une méthode où toute la logique est effectuée à l'intérieur d'une boucle foreach qui itère sur le paramètre de la méthode:

public IEnumerable<TransformedNode> TransformNodes(IEnumerable<Node> nodes)
{
    foreach(var node in nodes)
    {
        // yadda yadda yadda
        yield return transformedNode;
    }
}

Dans ce cas, l'envoi d'une collection vide donne une collection vide, mais je me demande si c'est imprudent.

Ma logique ici est que si quelqu'un appelle cette méthode, alors il a l'intention de transmettre des données et ne transmettrait une collection vide à ma méthode que dans des circonstances erronées.

Devrais-je attraper ce comportement et lancer une exception, ou est-il préférable de renvoyer la collection vide?

Nick Udell
la source
43
Êtes-vous sûr que votre hypothèse "si quelqu'un appelle cette méthode, alors il a l'intention de transmettre des données" est correcte? Peut-être ne font-ils que passer ce qu'ils ont sous la main pour travailler avec le résultat.
SpaceTrucker
7
BTW: votre méthode de "transformation" est généralement appelée "carte", bien que dans .NET (et SQL, d'où est nommé le nom .NET), elle est communément appelée "select". "Transformer" est ce que C ++ appelle cela, mais ce n'est pas un nom commun que l'on pourrait reconnaître.
Jörg W Mittag
25
Je lancerais si la collection est, nullmais pas si elle est vide.
CodesInChaos
21
@ NickUdell - Je passe tout le temps des collections vides dans mon code. Il est plus facile que le code se comporte de la même manière que d'écrire une logique de branchement spéciale dans les cas où je ne pouvais rien ajouter à ma collection avant de passer à autre chose.
theMayer
7
Si vous vérifiez et générez une erreur si la collection est vide, vous forcez également votre appelant à vérifier et à ne pas transmettre une collection vide à votre méthode. Les validations doivent être effectuées sur (et uniquement sur) les limites du système. Si des collections vides vous dérangent, vous devriez déjà les avoir vérifiées bien avant qu’elles n’aient atteint les méthodes utilitaires. Vous avez maintenant deux chèques redondants.
Lie Ryan

Réponses:

175

Les méthodes utilitaires ne doivent pas jeter sur des collections vides. Vos clients API vous détesteraient pour cela.

Une collection peut être vide. une "collection qui ne doit pas être vide" est conceptuellement une chose beaucoup plus difficile à travailler.

La transformation d'une collection vide a un résultat évident: la collection vide. (Vous pouvez même économiser des déchets en retournant le paramètre lui-même.)

Il existe de nombreuses circonstances dans lesquelles un module conserve des listes d'éléments remplis ou non. Devoir vérifier la vacuité avant chaque appel transformest ennuyeux et a le potentiel de transformer un algorithme simple et élégant en désordre odieux.

Les méthodes d’utilité devraient toujours s’efforcer d’être libérales dans leurs intrants et conservatrices dans leurs extrants.

Pour toutes ces raisons, pour l'amour de Dieu, gérez correctement la collection vide. Rien n'est plus exaspérant qu'un module d'assistance qui pense savoir ce que vous voulez mieux que vous.

Kilian Foth
la source
14
+1 - (plus si je pouvais). Je pourrais même aller jusqu'à fusionner nulldans la collection vide. Les fonctions avec des limitations implicites sont pénibles.
Telastyn
6
"Non, vous ne devriez pas" ... ne devriez pas les accepter ou ne pas lancer une exception?
Ben Aaronson
16
@Andy - ce ne serait pas des méthodes utilitaires cependant. Ce sont des méthodes commerciales ou un comportement concret similaire.
Telastyn
38
Je ne pense pas que recycler la collection vide pour un retour est une bonne idée. Vous ne sauvez qu'un tout petit peu de mémoire; et cela pourrait entraîner un comportement inattendu chez l'appelant. Exemple légèrement artificiel: Populate1(input); output1 = TransformNodes(input); Populate2(input); output2 = TransformNodes(input); si Populate1 laissait la collection vide et que vous la renvoyiez pour le premier appel TransformNodes, output1 et input seraient la même collection. ensemble d'entrées dans output2.
Dan Neely
6
@Andy Malgré tout, si l'argument ne doit jamais être vide - s'il est erroné de ne pas traiter de données -, j'estime qu'il incombe à l' appelant de vérifier cela, lorsqu'il rassemble cet argument. Non seulement cette méthode est-elle plus élégante, mais cela signifie également que l'on peut générer un meilleur message d'erreur (meilleur contexte) et le relever de manière rapide. Cela n'a pas de sens pour une classe en aval de vérifier les invariants de l'appelant ...
Andrzej Doyle
26

Je vois deux questions importantes qui déterminent la réponse à cette question:

  1. Votre fonction peut-elle renvoyer quelque chose de significatif et de logique lorsqu'elle passe une collection vide (y compris null )?
  2. Quel est le style de programmation général dans cette application / bibliothèque / équipe? (Plus précisément, comment allez-vous?)

1. Retour significatif

En gros, si vous pouvez retourner quelque chose de significatif, ne lancez pas d'exception. Laissez l'appelant gérer le résultat. Donc, si votre fonction ...

  • Compte le nombre d'éléments de la collection, renvoie 0. C'était simple.
  • Recherche des éléments correspondant à des critères particuliers, renvoie une collection vide. S'il vous plaît ne jetez rien. L'appelant peut disposer d'une vaste collection de collections, certaines vides, d'autres non. L'appelant veut tous les éléments correspondants dans n'importe quelle collection. Les exceptions ne font que rendre la vie de l'appelant plus difficile.
  • Cherche le plus grand / le plus petit / le mieux adapté aux critères de la liste. Oops. Selon la question de style, vous pouvez générer une exception ici ou renvoyer null . Je déteste null (très quelqu'un de PF) mais c'est sans doute plus significatif ici et cela vous permet de réserver des exceptions pour des erreurs inattendues dans votre propre code. Si l'appelant ne vérifie pas la valeur null, une exception relativement claire en résultera de toute façon. Mais vous leur laissez ça.
  • Demande le nième élément de la collection ou les n premiers / derniers éléments. C'est le meilleur cas pour une exception pour le moment et celui qui est le moins susceptible de causer de la confusion et des difficultés à l'appelant. Un cas peut toujours être créé pour null si votre équipe et vous-même avez l'habitude de vérifier cela, pour toutes les raisons données dans le point précédent, mais c'est le cas le plus valable à ce jour pour le lancement d'une exception DudeYou Know YouShouldCheck TheSizeFirst . Si votre style est plus fonctionnel, alors null ou lisez la réponse à ma réponse Style .

En général, mon biais de PF m'indique «rendre quelque chose de significatif» et null peut avoir une signification valable dans ce cas.

2. style

Votre code général (ou le code du projet ou celui de l’équipe) favorise-t-il un style fonctionnel? Si non, des exceptions seront attendues et traitées. Si oui, envisagez de renvoyer un type d'option . Avec un type d'option, vous renvoyez une réponse significative ou Aucune / Rien . Dans mon troisième exemple, ci-dessus, Nothing ne serait pas une bonne réponse en style de PF. Le fait que la fonction renvoie clairement à l'appelant un type d'option plus qu'une réponse significative peut ne pas être possible et que l'appelant doit être prêt à y faire face. Je pense que cela donne plus de choix à l'appelant (si vous pardonnez le jeu de mots).

F # est l' endroit où tous les frais .Net enfants font ce genre de chose , mais C # ne soutenir ce style.

tl; dr

Conservez les exceptions pour les erreurs inattendues dans votre propre chemin de code, sans entrée totalement prévisible (et légale) de la part de quelqu'un d’autre.

itsbruce
la source
"Meilleur ajustement", etc.: vous pouvez avoir une méthode qui trouve le meilleur objet dans une collection qui correspond à un critère. Cette méthode poserait le même problème pour les collections non vides, car rien ne correspondait au critère; vous n'avez donc pas à vous soucier des sélections vides.
gnasher729
1
Non, c'est pourquoi j'ai dit «le meilleur ajustement» et non pas les «correspondances»; la phrase implique l'approximation la plus proche, pas une correspondance exacte. La plus grande / la plus petite des options aurait dû être un indice. Si seul le «meilleur ajustement» a été demandé, toute collection comprenant un ou plusieurs membres devrait pouvoir renvoyer un membre. S'il n'y a qu'un seul membre, c'est la meilleure solution.
Itsbruce
1
Retourner "null" dans la plupart des cas est pire que de lancer une exception. Cependant, renvoyer une collection vide est significatif.
Ian
1
Pas si vous devez retourner un seul article (min / max / tête / queue). Quant à null, comme je l’ai dit, je le déteste (et préfère les langues qui ne l’ont pas), mais si une langue l’a, vous devez être capable de le gérer et de coder de la sorte. Selon les conventions de codage locales, cela peut être approprié.
Itsbruce
Aucun de vos exemples n'a rien à voir avec une entrée vide. Ce ne sont que des cas particuliers de situations où la réponse pourrait être "rien". Ce qui à son tour devrait répondre à la question du PO sur comment "gérer" la collection vide.
Djechlin
18

Comme toujours, ça dépend.

Est-il important que la collection soit vide?
La plupart des codes de traitement des collections diraient probablement "non"; une collection peut contenir n'importe quel nombre d'éléments, y compris zéro.

Maintenant, si vous avez une sorte de collection où il est "invalide" de ne pas contenir d’éléments, alors c’est une nouvelle exigence et vous devez décider quoi faire à ce sujet.

Empruntez une logique de test du monde de la base de données: test pour zéro élément, un élément et deux éléments. Cela convient aux cas les plus importants (élimination de toute condition de jointure interne ou cartésienne mal formée).

Phill W.
la source
Et s'il n'est pas valide de ne pas avoir d'éléments dans une collection, alors, idéalement, l'argument ne serait pas un java.util.Collection, mais une com.foo.util.NonEmptyCollectionclasse personnalisée , qui peut conserver systématiquement cet invariant et vous empêcher de passer à un état invalide.
Andrzej Doyle
3
Cette question est étiquetée [c #]
Nick Udell
@ NickUdell C # ne prend-il pas en charge la programmation orientée objet ou le concept d'espaces de noms imbriqués? TIL.
John Dvorak
2
Cela fait. Mon commentaire était une clarification, car Andrzej devait avoir été confus (sinon, pourquoi s’engageraient-ils dans l’effort de spécifier des noms espacés pour une langue à laquelle cette question ne fait pas référence?).
Nick Udell
-1 pour souligner le "ça dépend" plus que le "presque certainement oui". Sans blague - communiquer la mauvaise chose fait du tort ici.
Djechlin
11

Pour une bonne conception, acceptez autant que possible les variations de vos entrées. Les exceptions doivent seulement être levée lorsque (entrée inacceptable est des erreurs présentées ou inattendues se produire lors du traitement) et le programme ne peut pas continuer de manière prévisible en conséquence.

Dans ce cas, une collection vide doit être attendue et votre code doit le gérer (ce qui est déjà le cas). Ce serait une violation de tout ce qui est bon si votre code lançait une exception ici. Cela reviendrait à multiplier 0 par 0 en mathématiques. Il est redondant, mais absolument nécessaire pour que cela fonctionne comme il le fait.

Passons maintenant à l'argument de collection null. Dans ce cas, une collection nulle est une erreur de programmation: le programmeur a oublié d'attribuer une variable. Dans ce cas, une exception pourrait être levée, car vous ne pouvez pas le traiter de manière significative dans une sortie, et tenter de le faire introduirait un comportement inattendu. Cela équivaudrait à une division par zéro en mathématiques - cela n'a aucun sens.

laMayer
la source
Votre déclaration audacieuse est un très mauvais conseil en toute généralité. Je pense que c'est vrai ici, mais vous devez expliquer en quoi cette situation est spéciale. (C'est un mauvais conseil en général, car cela favorise les échecs silencieux.)
djechlin
@AAA - nous n'écrivons manifestement pas un livre sur la conception de logiciels, mais je ne pense pas que ce soit un mauvais conseil si vous comprenez vraiment ce que je dis. Mon point est que si une mauvaise entrée produit une sortie ambiguë ou arbitraire, vous devez lever une exception. Dans les cas où la sortie est totalement prévisible (comme c'est le cas ici), le lancement d'une exception serait alors arbitraire. La clé est de ne jamais prendre de décisions arbitraires dans votre programme, car c’est de là que vient le comportement imprévisible.
theMayer
Mais c'est votre première phrase et la seule en surbrillance ... personne ne comprend encore ce que vous dites. (Je n'essaie pas d'être trop agressif ici, nous sommes juste ici pour apprendre à écrire et à communiquer et je pense que la perte de précision d'un point clé est néfaste.)
djechlin
10

Il est beaucoup plus difficile de voir la bonne solution lorsque vous regardez votre fonction de manière isolée. Considérez votre fonction comme une partie d’ un problème plus vaste . Une solution possible à cet exemple ressemble à ceci (en Scala):

input.split("\\D")
.filterNot (_.isEmpty)
.map (_.toInt)
.filter (x => x >= 1000 && x <= 9999)

Tout d'abord, vous divisez la chaîne par des chiffres non numériques, filtrez les chaînes vides, convertissez les chaînes en entiers, puis filtrez pour ne conserver que les nombres à quatre chiffres. Votre fonction pourrait être la map (_.toInt)dans le pipeline.

Ce code est assez simple car chaque étape du pipeline ne gère qu'une chaîne vide ou une collection vide. Si vous insérez une chaîne vide au début, vous obtenez une liste vide à la fin. Vous n'êtes pas obligé de vous arrêter et de rechercher nullune exception après chaque appel.

Bien entendu, cela suppose qu'une liste de sortie vide n'a pas plus d'un sens. Si vous devez différencier une sortie vide provoquée par une entrée vide de celle générée par la transformation elle-même, cela change complètement les choses.

Karl Bielefeldt
la source
+1 pour un exemple extrêmement efficace (manque d'autres bonnes réponses).
Djechlin
2

Cette question concerne en réalité des exceptions. Si vous le regardez de cette façon et que vous ignorez la collection vide en tant que détail d'implémentation, la réponse est simple:

1) Une méthode doit lever une exception quand elle ne peut pas continuer: soit incapable de faire la tâche désignée, soit retourne la valeur appropriée.

2) Une méthode devrait intercepter une exception lorsqu'elle est capable de continuer malgré l'échec.

Ainsi, votre méthode d'assistance ne devrait pas être "utile" et lever une exception à moins qu'elle ne puisse pas faire son travail avec une collection vide. Laissez l'appelant déterminer si les résultats peuvent être traités.

Que cela retourne une collection vide ou null, c'est un peu plus difficile mais pas beaucoup: les collections nullables doivent être évitées si possible. Le but d’une collection nullable serait d’indiquer (comme en SQL) que vous n’avez pas l’information - une collection d’enfants, par exemple, peut être nulle si vous ne savez pas si quelqu'un en a une, mais vous n'en avez pas. sais qu'ils ne le font pas. Mais si cela est important pour une raison quelconque, il vaut probablement la peine d'être suivi par une variable supplémentaire.

Jmoreno
la source
1

La méthode est nommée TransformNodes. Dans le cas d'une collection vide en entrée, récupérer une collection vide est naturel et intuitif, et prend tout son sens mathématique.

Si la méthode était nommée Maxet conçue pour renvoyer l'élément maximum, il serait alors naturel de jeter NoSuchElementExceptionune collection vide, car le maximum de rien n'a aucun sens mathématique.

Si la méthode a été nommée JoinSqlColumnNameset conçue pour renvoyer une chaîne où les éléments sont joints par une virgule à utiliser dans les requêtes SQL, il serait alors judicieux de créer IllegalArgumentExceptionune collection vide, car l'appelant finirait par obtenir une erreur SQL s'il utilisait la chaîne dans une requête SQL directement sans autres vérifications, et au lieu de rechercher une chaîne vide renvoyée, il aurait plutôt dû rechercher une collection vide.

Arrêtez le mal en cours à Monica
la source
Maxde rien est souvent l'infini négatif.
Djechlin
0

Revenons en arrière et utilisons un exemple différent qui calcule la moyenne arithmétique d'un tableau de valeurs.

Si le tableau d'entrée est vide (ou nul), pouvez-vous raisonnablement répondre à la demande de l'appelant? Non. Quelles sont vos options? Eh bien, vous pourriez:

  • présenter / retourner / jeter une erreur. en utilisant la convention de votre base de code pour cette classe d'erreur.
  • documenter qu'une valeur telle que zéro sera retournée
  • document qu'une valeur invalide désignée sera retournée (par exemple, NaN)
  • documenter qu'une valeur magique sera retournée (par exemple, un minimum ou un maximum pour le type ou une valeur indicative)
  • déclarer le résultat n'est pas spécifié
  • déclarer l'action non définie
  • etc.

Je leur dis de leur donner l'erreur s'ils vous ont donné une entrée invalide et que la demande ne peut pas être complétée. Je veux dire une erreur dès le premier jour pour qu'ils comprennent les exigences de votre programme. Après tout, votre fonction n'est pas en mesure de répondre. Si l'opération peut échouer (par exemple, copier un fichier), votre API doit leur indiquer une erreur à laquelle ils peuvent remédier.

Cela peut donc définir comment votre bibliothèque traite les requêtes mal formées et les requêtes susceptibles d’échouer.

Il est très important que votre code soit cohérent dans la façon dont il traite ces classes d’erreurs.

La catégorie suivante consiste à décider de la manière dont votre bibliothèque traite les requêtes non-sens. Pour en revenir à un exemple similaire à la vôtre - Utilisons une fonction qui détermine si un fichier existe sur un chemin: bool FileExistsAtPath(String). Si le client passe une chaîne vide, comment gérez-vous ce scénario? Que diriez-vous d'un tableau vide ou nul passé à void SaveDocuments(Array<Document>)? Choisissez votre bibliothèque / base de code et soyez cohérent. Il m'est arrivé de considérer ces erreurs comme des erreurs et d’interdire aux clients de faire des requêtes insensées en les signalant comme des erreurs (via une assertion). Certaines personnes vont fortement résister à cette idée / action. Je trouve cette détection d'erreur très utile. C'est très bien pour localiser les problèmes dans les programmes - avec un bon endroit pour le programme incriminé. Les programmes sont beaucoup plus clairs et corrects (considérez l’évolution de votre base de code), et ne gravez pas de cycles dans des fonctions qui ne font rien. Le code est plus petit / plus propre de cette manière, et les vérifications sont généralement transférées aux emplacements où le problème peut être introduit.

Justin
la source
6
Pour votre dernier exemple, ce n'est pas le travail de cette fonction de déterminer ce qui est absurde. Par conséquent, une chaîne vide doit renvoyer false. En effet, c’est l’approche exacte que File.Exists adopte.
theMayer
@ rmayer06 c'est son travail. la fonction référencée autorise des chaînes vides, des chaînes vides, vérifie la présence de caractères non valides dans la chaîne, effectue une troncature de chaîne, peut-être doit passer des appels au système de fichiers, peut-être interroger l’environnement, etc. coût élevé, et ils brouillent définitivement les lignes de la correction (IMO). J'ai vu beaucoup de if (!FileExists(path)) { ...create it!!!... }bugs - dont beaucoup seraient capturés avant d'être validés, les lignes de la rectitude n'étant pas floues.
Justin
2
Je serais d'accord avec vous, si le nom de la fonction était File.ThrowExceptionIfPathIsInvalid, mais qui, dans son esprit, appellerait une telle fonction?
theMayer
Une moyenne de 0 éléments va déjà renvoyer NaN ou émettre une exception de division par zéro, uniquement en raison de la définition de la fonction. Un fichier avec un nom qui ne peut pas exister, n’existera pas ou déclenchera déjà une erreur. Il n'y a aucune raison impérieuse de traiter ces cas spécialement.
cHao
On pourrait même soutenir que le concept même de vérifier l'existence d'un fichier avant de le lire ou de l'écrire est de toute façon défectueux. (Il y a une condition de concurrence inhérente.) Mieux vaut continuer et essayer d'ouvrir le fichier, pour le lire ou avec les paramètres de création uniquement si vous écrivez. Il échouera déjà si le nom de fichier est invalide ou n'existe pas (ou existe en cas d'écriture).
cHao
-3

En règle générale, une fonction standard doit être capable d'accepter la liste la plus large d'entrées et de donner un retour sur celle-ci. Il existe de nombreux exemples dans lesquels des programmeurs utilisent des fonctions d'une manière que le concepteur n'avait pas planifiée. devrait pouvoir accepter non seulement des collections vides, mais également un large éventail de types d'entrée et renvoyer gracieusement les retours, même s'il s'agit d'un objet d'erreur, quelle que soit l'opération effectuée sur l'entrée ...

Clément Mark-Aaba
la source
Le concepteur a absolument tort de supposer qu'une collection sera toujours passée et d'aller directement à une itération de ladite collection. Une vérification doit être effectuée pour s'assurer que l'argument reçu est conforme au type de données attendu ...
Clement Mark-Aaba
3
"large gamme de types d'entrée" n'a pas d'importance ici. La question est étiquetée c # , un langage fortement typé. Autrement dit, compilateur garantit que l' entrée est une collection
moucheron
Veuillez google "règle de base", ma réponse a été une mise en garde générale qu'en tant que programmeur, vous faites de la place pour des événements imprévus ou erronés, l'un de ceux-ci peut être passé à tort des arguments de fonction ...
Clement Mark-Aaba
1
C'est faux ou tautologique. writePaycheckToEmployeene devrait pas accepter un nombre négatif en entrée ... mais si "feedback" signifie "fait tout ce qui pourrait se passer ensuite", alors oui, chaque fonction fera quelque chose qu'elle fera ensuite.
Djechlin