Pourquoi Optional.get () sans appeler isPresent () est-il mauvais, mais pas iterator.next ()?

Lorsque j'utilise la nouvelle API Java8 streams pour trouver un élément spécifique dans une collection, j'écris du code comme ceci:

    String theFirstString = myCollection.stream()
            .findFirst()
            .get();

Ici, IntelliJ avertit que get () est appelé sans vérifier isPresent () en premier.

Cependant, ce code:

    String theFirstString = myCollection.iterator().next();

... ne donne aucun avertissement.

Y a-t-il une différence profonde entre les deux techniques, qui rend l'approche de streaming plus "dangereuse" d'une certaine manière, qu'il est crucial de ne jamais appeler get () sans appeler isPresent () en premier? En parcourant Google, je trouve des articles qui parlent de la négligence des programmeurs avec Optional <> et supposent qu'ils peuvent appeler get () à tout moment.

Le fait est que j'aime recevoir une exception aussi près que possible de l'endroit où mon code est bogué, qui dans ce cas serait l'endroit où j'appelle get () quand il n'y a aucun élément à trouver. Je ne veux pas avoir à écrire des essais inutiles comme:

    Optional<String> firstStream = myCollection.stream()
            .findFirst();
    if (!firstStream.isPresent()) {
        throw new IllegalStateException("There is no first string even though I totally expected there to be! <sarcasm>This exception is much more useful than NoSuchElementException</sarcasm>");
    }
    String theFirstString = firstStream.get();

... sauf s'il existe un danger à provoquer des exceptions à get () que je ne connais pas?

Après avoir lu la réponse de Karl Bielefeldt et un commentaire de Hulk, j'ai réalisé que mon code d'exception ci-dessus était un peu maladroit, voici quelque chose de mieux:

    String errorString = "There is no first string even though I totally expected there to be! <sarcasm>This exception is much more useful than NoSuchElementException</sarcasm>";
    String firstString = myCollection.stream()
            .findFirst()
            .orElseThrow(() -> new IllegalStateException(errorString));

Cela semble plus utile et deviendra probablement naturel dans de nombreux endroits. J'ai toujours l'impression de vouloir choisir un élément dans une liste sans avoir à gérer tout cela, mais cela pourrait juste être que je dois m'habituer à ce genre de constructions.

Tout d'abord, considérez que le contrôle est complexe et probablement relativement long à faire. Cela nécessite une analyse statique et il peut y avoir un peu de code entre les deux appels.

Deuxièmement, l'utilisation idiomatique des deux constructions est très différente. Je programme à plein temps en Scala, qui utilise Optionsbeaucoup plus fréquemment que Java. Je ne me souviens pas d'une seule instance où j'avais besoin d'utiliser un .get()sur un Option. Vous devriez presque toujours renvoyer le Optionaldepuis votre fonction, ou utiliserorElse() place. Ce sont des façons bien supérieures de gérer les valeurs manquantes que de lever des exceptions.

Étant donné qu'il .get()devrait rarement être appelé en utilisation normale, il est logique qu'un IDE lance une vérification complexe lorsqu'il voit un .get()pour voir s'il a été utilisé correctement. En revanche,iterator.next() est l'utilisation appropriée et omniprésente d'un itérateur.

En d'autres termes, il ne s'agit pas de l'un d'être mauvais et de l'autre non, il s'agit de l'un étant un cas commun qui est fondamental pour son fonctionnement et est très susceptible de lever une exception lors des tests de développement, et l'autre étant rarement utilisé cas qui peut ne pas être suffisamment exercé pour lever une exception lors des tests du développeur. Ce sont les types de cas dont vous voulez que les IDE vous avertissent.

La classe facultative est destinée à être utilisée lorsque l'on ne sait pas si l'élément qu'elle contient est présent ou non. L'avertissement existe pour informer les programmeurs qu'il existe un chemin de code possible supplémentaire qu'ils peuvent être en mesure de gérer correctement. L'idée est d'éviter que des exceptions soient levées. Le cas d'utilisation que vous présentez n'est pas ce pour quoi il est conçu, et donc l'avertissement n'est pas pertinent pour vous - si vous voulez réellement qu'une exception soit levée, allez-y et désactivez-la (bien que seulement pour cette ligne, pas globalement - vous devriez être prendre la décision de traiter ou non le cas non présent au cas par cas, et l'avertissement vous rappellera de le faire.

Il est possible qu'un avertissement similaire soit généré pour les itérateurs. Cependant, cela n'a tout simplement jamais été fait, et en ajouter un maintenant entraînerait probablement trop d'avertissements dans les projets existants pour être une bonne idée.

Notez également que lever votre propre exception peut être plus utile qu'une simple exception par défaut, car inclure une description de l'élément qui devait exister mais ne peut pas être très utile pour le débogage à un stade ultérieur.

Je suis d'accord qu'il n'y a pas de différence inhérente dans ces derniers, cependant, je voudrais souligner un plus gros défaut.

Dans les deux cas, vous avez un type qui fournit une méthode dont le fonctionnement n'est pas garanti et vous êtes censé appeler une autre méthode avant cela. Que votre IDE / compilateur / etc vous prévienne d'un cas ou de l'autre, cela dépend simplement s'il prend en charge ce cas et / ou si vous l'avez configuré pour le faire.

Cela étant dit, les deux morceaux de code sont des odeurs de code. Ces expressions font allusion aux défauts de conception sous-jacents et produisent un code fragile.

Vous avez raison de vouloir échouer rapidement, bien sûr, mais la solution, du moins pour moi, ne peut pas simplement être de hausser les épaules et de jeter vos mains en l'air (ou une exception sur la pile).

Votre collection peut être connue pour être non vide pour l'instant, mais tout ce sur quoi vous pouvez vraiment compter est son type: Collection<T> quoi - et cela ne vous garantit pas le non-vide. Même si vous savez maintenant qu'il n'est pas vide, une exigence modifiée peut entraîner une modification initiale du code et tout à coup, votre code est frappé par une collection vide.

Maintenant, vous êtes libre de repousser et de dire aux autres que vous étiez censé obtenir une liste non vide. Vous pouvez être heureux de trouver rapidement cette «erreur». Néanmoins, vous avez un logiciel cassé et devez changer votre code.

Comparez cela avec une approche plus pragmatique: puisque vous obtenez une collection et qu'elle peut éventuellement être vide, vous vous forcez à traiter ce cas en utilisant facultatif # map, #orElse ou toute autre de ces méthodes, sauf #get.

Le compilateur fait autant de travail que possible pour vous de sorte que vous pouvez compter autant que possible sur le contrat de type statique pour obtenir un Collection<T>. Lorsque votre code ne viole jamais ce contrat (par exemple via l'une de ces deux chaînes d'appel malodorantes), votre code est beaucoup moins fragile aux conditions environnementales changeantes.

Dans le scénario ci-dessus, une modification produisant une collection d'entrée vide serait sans conséquence pour votre code. C'est juste une autre entrée valide et donc toujours gérée correctement.

Le coût de cela est que vous devez investir du temps dans de meilleures conceptions, par opposition à a) risquer un code fragile ou b) compliquer inutilement les choses en lançant des exceptions.

Sur une note finale: puisque tous les IDE / compilateurs ne mettent pas en garde contre les appels iterator (). Next () ou optional.get () sans les vérifications préalables, ce code est généralement signalé dans les revues. Pour ce que ça vaut, je ne laisserais pas un tel code permettre de passer un examen et d'exiger une solution propre à la place.