Retour booléen de set.add () in if conditionnel?

15

L'opérateur add de la classe set renvoie un booléen qui est vrai si l'élément (qui doit être ajouté) n'était pas déjà là, et faux sinon. Est en train d'écrire

if (set.add(entry)) {
    //do some more stuff
}

considéré comme un bon style en termes d'écriture de code propre? Je me demande puisque vous faites deux choses à la fois. 1) ajouter l'élément et 2) vérifier si l'élément existait.

java coding-style clean-code Andreas Braun
la source
6
Vous parlez de la norme java.util.Set, qui revient vrai addlorsque l'élément n'était pas déjà là, non?
user2357112 prend en charge Monica
1
Je considérerais généralement le test opposé:if (!set.add(entry)) {// entry already present, possibly a case you want to handle}
njzk2
Y a-t-il quelque chose de mal à faire deux choses à la fois?
user207421
@ user2357112 oui, c'est vrai.
Andreas Braun

Réponses:

19

Oui, ça l'est.

Le point habituel pour qu'une opération retourne une valeur booléenne est que vous pouvez l'utiliser pour prendre des décisions, c'est-à-dire dans une ifconstruction. La seule autre façon de réaliser ce potentiel serait de stocker la valeur de retour dans une variable, puis de la réutiliser immédiatement dans un if, ce qui est tout simplement idiot et certainement pas préférable à l'écriture if(operation()) { ... }. Alors allez-y et faites-le, nous ne vous jugerons pas (pour cela).

Kilian Foth
la source
Merci pour votre réponse. Comme @Niels vanReijmersdal le souligne dans sa réponse, cela rompt la séparation des commandes et des requêtes, n'est-ce pas? Ne pensez-vous pas que cela compte?
Andreas Braun
4
@AndreasBraun personnellement, je pense que la séparation des requêtes de commande est stupide. Il est souvent logique qu'une méthode effectue à la fois une action et renvoie une valeur liée à cette action. L'application d'une séparation artificielle entre les deux conduit à un code inutilement verbeux.
1
@ dan1111: en effet. En outre, la «séparation des commandes et des requêtes» conduit à la «vérification puis action» et à des modèles anti similaires, qui peuvent se briser dans des contextes multithreads. Il est plutôt étrange d'annoncer une telle séparation, alors que le reste du monde essaie de construire des opérations fusionnées atomiques pour des solutions SMP robustes et efficaces en même temps…
Holger
2
@ Jörg W Mittag: page 759 de quoi? Puisqu'une opération atomique est intrinsèquement immunisée contre l'anti-modèle "check then act", il ne semble pas avantageux de la diviser juste pour lire un "chapitre entier" de tout ce qui doit être découvert, comment rendre le thread de construction sûr encore. Puisque vous n'avez cité aucun argument réel, je n'ai pas d '«objections spécifiques à ces arguments».
Holger
1
@ Jörg W Mittag: Eh bien, je parle de la réponse sur cette page, décourageant l'utilisation en Set.addtant qu'opération unique sans nommer d'alternative. Si l'opération envisagée consiste à ajouter un seul élément et à savoir s'il a été ajouté, il n'y a pas besoin d'une alternative plus puissante qui complique encore l'opération sans bénéfice. J'espère que vous savez que Set.addc'est une méthode JRE intégrée qui peut être utilisée sans étudier d'abord un livre de> 700 pages.
Holger
12

Je dirais que ce n'est pas aussi propre que possible, car cela oblige le responsable à déjà savoir ou à rechercher ce que signifie la valeur de retour. Cela signifie-t-il que la valeur existait déjà, n'existait pas déjà, a été insérée avec succès? Si vous ne l'utilisez pas beaucoup, vous ne le saurez pas, et même si vous le faites, c'est beaucoup plus de charge mentale.

Je préférerais ce qui suit:

boolean added = set.add(entry);

if (added) {
    //do some more stuff
}

Oui, un peu plus verbeux, mais le compilateur devrait générer à peu près exactement le même bytecode, et même les personnes qui n'ont pas utilisé les ensembles Java depuis des années peuvent suivre la logique sans rien rechercher.

Karl Bielefeldt
la source
2
addrenvoie true si l'élément n'était pas déjà là, pas s'il l'était. (Le libellé de la question est trompeur.)
user2357112 prend en charge Monica
9
La recherche d'une méthode est moins lourde que d'essayer de comprendre l'intention du développeur d'origine pour une variable redondante déclarée en dehors de la portée où elle est utilisée. Dans tous les IDE Java modernes, un développeur peut placer le pointeur de sa souris sur une méthode et accéder instantanément à sa documentation. Les bons développeurs le font constamment lorsqu'ils travaillent avec une API inconnue.
Kevin Krumwiede
9
J'appuie @Holger. Si vous ne le savez pas Set.add, vous êtes inexpérimenté et vous devriez chercher ces méthodes pour les apprendre et devenir plus expérimentées.
Rétablir Monica le
7
notAlreadyPresentn'est pas la meilleure formulation. J'utiliserais addedet m'attendrais à ce que le lecteur sache pourquoi une valeur ne serait pas ajoutée à un ensemble.
njzk2
3
@JustinTime: L'utilisation de commentaires pour décrire ce que fait le code est largement considérée comme une mauvaise pratique. Votre code doit être auto-descriptif. Dans une révision de code, je signalerais votre exemple comme inacceptable.
BlueRaja - Danny Pflughoeft
8

Si true signifie succès, alors c'est un bon code clair.

Il existe une convention répandue selon laquelle une fonction ou une méthode renvoie true (ou quelque chose qui s'évalue à true) en cas de succès. Tant que votre code suit cela, je pense que mettre la méthode au conditionnel est très bien.

Un code comme celui-ci est inutilement encombré à mon avis:

boolean frobulate_succeeded = thing.frobulate();

if (frobulate_succeeded) {
    ...
}

C'est comme si vous vous répétiez.

Cependant, la question est ambiguë sur la signification de la valeur de retour. Vous dites "un booléen qui indique si l'élément ajouté existait déjà", ce qui pourrait impliquer que true signifie que l'élément existait (et que l'ajout ne s'est pas produit). Si tel est le cas, je changerais idéalement le comportement de retour de la méthode pour être plus conventionnel. Si ce n'est pas possible, j'ajouterais une variable intermédiaire supplémentaire qui vous permet d'étiqueter clairement le résultat du retour dans votre code (comme suggéré par d'autres).


la source
Que signifie le succès dans le contexte de l'ajout d'un élément à un ensemble? Ne pas lever d'exception signifie succès; vrai contre faux signifie quelque chose de plus spécifique dans ce contexte.
Rétablir Monica le
@ Solomonoff'sSecret Dans ce cas, une exception signifie que l'ensemble a refusé d'ajouter l'élément, falsesignifie que l'élément n'a pas été ajouté car il était déjà contenu et truesignifie que l'élément n'était pas déjà contenu. Comme add()ajoute l'élément à l'ensemble si l'ensemble ne le contient pas déjà, truecela signifie donc que l'élément a été ajouté avec succès à l'ensemble.
Justin Time - Reinstate Monica
@JustinTime Vous ajoutez vos propres jugements de valeur aux deux affaires. Une autre interprétation est que le succès est que l'élément est dans l'ensemble lorsque la méthode revient. Si l'élément était déjà dans l'ensemble, addréussit sans rien faire. Si l'élément n'était pas déjà dans l'ensemble, addréussit en ajoutant l'élément à l'ensemble. Quelle interprétation est correcte est arbitraire. La définition moins arbitraire du succès est celle du langage: la méthode réussit si elle revient normalement.
Rétablir Monica le
1
La définition la moins arbitraire du succès est que la méthode a exécuté avec succès la tâche qu'elle avait l'intention d'effectuer. Si j'ai une méthode firstLessThanSecond(int l, int r), et il retourne truesi l > rou falsesi l <= r, alors cette méthode est pas réussie, même si elle retourne normalement.
Justin Time - Rétablir Monica le
1
@JustinTime addest une abréviation grave du contrat. La documentation commence par "Ajoute l'élément spécifié à cet ensemble s'il n'est pas déjà présent", qui vérifie si l'élément était déjà dans l'ensemble. Vous êtes libre d'interpréter la valeur de retour comme vous le souhaitez, mais en fin de compte, c'est juste votre interprétation. Et dans votre deuxième exemple, la méthode évalue si une condition est vraie. Si la condition est fausse, la méthode n'a certainement pas échoué, car elle a réussi à évaluer le conditionnel.
Rétablir Monica le
2

Je dirais que c'est très semblable à C. La plupart du temps, je préférerais avoir une variable nommée de manière descriptive pour un résultat de mutation, et aucune mutation ne se produisant dans une ifcondition.

Un compilateur éliminera cette variable si elle est immédiatement réutilisée. Un humain aura plus de facilité à lire la source; pour moi, c'est plus important.

Si quelqu'un doit étendre la condition en ajoutant une clause and/ orà la condition if, il peut finir par ne pas appeler .add()dans certains cas en raison d'une évaluation de court-circuit. Sauf si un court-circuit est spécifiquement prévu, cela peut devenir un bogue.

9000
la source
Si j'écris, if (set.contains(entry)){set.add(entry); //do more stuff}est-ce aussi éliminé par le compilateur?
Andreas Braun
1
@AndreasBraun: Je ne pense pas; le compilateur n'a aucune idée du lien sémantique entre containset add. En outre, il effectue un double travail de recherche entrydans l'ensemble; cela peut jouer un rôle pour les très grands ensembles et les boucles très légères.
9000
1
Je suppose donc que vous préférez ne pas écrire if (set.contains(entry)){set.add(entry); //do more stuff}mais plutôt aller avec, par exemple, la réponse de Karl Bielefeldt?
Andreas Braun
@AndreasBraun: exactement (a voté pour cette réponse).
9000
1
un bon point. Les mutations dans les ifs sont délicates, car un futur développeur peut avoir d'autres conditions avec des courts-circuits.
njzk2
0

Votre code semble rompre la séparation des requêtes de commande . Ceci est discuté dans le livre Clean Code et la vidéo Function Structure . Du point de vue du Clean Code, je pense que ce n'est pas considéré comme un bon style.

«Un code propre est simple et direct. Un code propre se lit comme une prose bien écrite. Un code propre n'obscurcit jamais l'intention du concepteur mais est plutôt plein d'abstractions nettes et de lignes de contrôle simples.

- Grady Booch, auteur d'Analysis Oriented Object and Design with Applications »

Pour moi, l'intention de votre code n'est pas claire. Est-ce que si les deux sont exécutés lorsque l'entrée est ajoutée avec succès, ou aussi quand elle existe déjà? Que add()revient-il? l'article? Le code d'erreur?

Niels van Reijmersdal
la source
2
Eh bien, c'est ce que je pensais aussi. Mais je pense aussi que @Kilian Foth a raison. Si je voulais séparer la requête et la commande, je devrais écrire if (set.contains(entry)){set.add(entry); //do more stuff}ce qui semble un peu idiot. Quel est votre avis là-dessus?
Andreas Braun
Je ne connais pas le domaine et le contexte de ce code pour suggérer un bon nom, mais je voudrais envelopper cela dans une fonction avec une intention claire. Par exemple: doesEntryExistOrCreateEntry (entrée), cela pourrait contenir votre logique contains () + add () et retourner un booléen. Question similaire ici: softwareengineering.stackexchange.com/questions/149708/… qui traite de cette pratique en dehors du code propre.
Niels van Reijmersdal
6
Je pense que la règle de séparation des requêtes de commande est stupide, en particulier lorsqu'elle est appliquée à un cas comme celui-ci, où une méthode à la fois exécute une action et renvoie l'état de réussite de l'action. Mais aussi, vous n'expliquez pas quelle est la règle ou ne fournissez pas beaucoup de justification. A voté pour ces raisons.
1
"Que renvoie add ()?" Je m'attends à ce que tout développeur Java qui effectue une révision de code connaisse l' CollectionAPI.
njzk2
3
D'accord avec @ dan1111. C'est particulièrement stupide parce que c'est le genre de choses qui rendent un terrain fertile pour les conditions de course.
Paul Draper