Pendant que je révisais du code, je suis tombé sur cet extrait de code.

List<User> users = /* Some code that initializes the list */;
users.stream()
     .filter(user -> user.getAddress().isPresent())
     .map(/* Some code */)
// And so on...

L'appel de la méthode user.getAddress() renvoie un Optional<Address>. Suivant la fameuse loi de Demeter (LoD), le code ci-dessus n'est pas propre. Cependant, je ne peux pas comprendre comment le refactoriser pour le rendre plus propre.

Comme première tentative pourrait être d'ajouter à la classe User une méthode hasAddress(), mais cette méthode surmonte le besoin d'avoir un Optional<Address>, IMO.

Comment refactoriser le code ci-dessus? Dans ce cas, vaut-il la peine de satisfaire la LD?

MODIFIER : j'ai manqué de spécifier que dans la méthode map je ne souhaite pas renvoyer l'adresse.

8
riccardo.cardin 17 nov. 2017 à 12:18

5 réponses

Meilleure réponse

Eh bien, vous l'avez assez bien résumé vous-même: si vous vouliez coupler plus librement en introduisant hasAddress(), pourquoi renvoyer un Optional.

En lisant ce que dit le LoD, il parle d'avoir des connaissances "limitées" sur les "étroitement liés" unités. Cela me semble une zone grise, mais pour aller plus loin, il mentionne également la règle "Un seul point". Pourtant, je serais d'accord avec les commentaires sur votre question qu'une vérification nulle (ou isPresent()) est tout à fait correcte (diable, une vraie vérification nulle n'a techniquement même pas besoin d'un point; P).

Si vous voulez vraiment encapsuler davantage, vous pouvez supprimer complètement le getAddress() et proposer à la place:

class User {
    private Optional<Address> address;

    boolean hasAddress() {
        return address.isPresent();
    }

    // still exposes address to the consumer, guard your properties
    void ifAddressPresent(Consumer<Address> then) {
        address.ifPresent(then::accept);
    }

    // does not expose address, but caller has no info about it
    void ifAddressPresent(Runnable then) {
        address.ifPresent(address -> then.run());
    }

    // really keep everything to yourself, allowing no outside interference
    void ifAddressPresentDoSomeSpecificAction() {
        address.ifPresent(address -> {
            // do this
            // do that
        });
    }
}

Mais encore une fois, comme l'ont souligné les commentateurs: cela en vaut-il la peine / est-il nécessaire? Toutes ces lois / principes sont rarement absolus et plus de directives que de dogmes. Dans ce cas, il peut s'agir d'équilibrer LoD et KISS.


En fin de compte, c'est à vous de décider si cet exemple spécifique bénéficie du déplacement des fonctionnalités de votre flux dans la classe User. Les deux sont valides et la lisibilité / la maintenabilité / la propreté dépendent:

  • le cas spécifique
  • comment ce code est-il exposé à d'autres modules
  • le nombre de méthodes déléguées dont vous auriez besoin dans la classe User
  • votre architecture (Si vous êtes dans une classe UserDao par exemple, voulez-vous vraiment déplacer la base de données accéder à votre classe User POJO? Le DAO n'est-il pas fait exactement dans ce but? Cela est-il considéré comme "étroitement lié" et permettrait une violation de la règle "Un seul point"?)
  • ...
3
Malte Hartwig 17 nov. 2017 à 12:45

Ce à quoi le LoD vous demande de réfléchir, c'est de savoir si la valeur de l'adresse doit ou non être donnée (c'est-à-dire demandée / consultée). Si la réponse est OUI - alors vous devez traiter le cas de valeur nulle ou vide; sinon, vous pouvez vous demander si l'existence d'une adresse peut être demandée ou non.

Donc, s'il est valide pour l'adresse à donner, alors le retour d'un facultatif gère la casse de la valeur nulle et peut être utilisé pour vérifier l'existence. Vérifier un optionnel pour «vide» me semble être la même chose que vérifier un entier pour 0 - vous n'accédez pas à un attribut d'un autre objet, juste un attribut d'un objet qui vous a été donné.

Si seule l'existence est autorisée à être connue, une méthode isPresent est probablement meilleure. Bien sûr, si vous traitez avec une valeur SQL, alors peut-être qu'un optionnel serait nécessaire pour gérer la valeur SQL NULL.

Sinon, pourquoi ce filtrage se produit-il ici?

0
Jay 20 nov. 2017 à 19:48

Je ne sais pas si c'est plus propre (car vous devez utiliser le fait que getAddress renvoie quand même une option), mais en Java 9, vous pouvez faire:

users.stream()
    .map(User::getAddress)
    .flatMap(Optional::stream)
    .map(/* Some code */)

Ou

users.stream()
    .flatMap(user -> user.getAddress().stream())
    .map(/* Some code */)
1
pron 17 nov. 2017 à 10:10

Je pense que vous avez déjà répondu à la question vous-même.

Vous avez deux cas d'utilisation distincts ici:

  1. Déterminer si l'utilisateur a une adresse
  2. Accédez à l'adresse utilisateur de manière sécurisée par zéro

Le premier cas d'utilisation peut être résolu en ajoutant la méthode hasAddress(), comme vous l'avez dit. Le deuxième cas d'utilisation peut être résolu en utilisant Optional pour envelopper l'adresse. Il n'y a rien de mal à cela.

0
Hugo Madureira 17 nov. 2017 à 12:21

Une autre approche serait de déplacer le calcul vers la classe d'utilisateurs.

Puisque l'utilisateur a l'adresse, il devrait être en mesure de répondre aux questions sur l'adresse et ne pas avoir à l'exposer. De cette façon, vous satisferiez le LoD

0
user3237183user3237183 29 déc. 2017 à 18:56
47347068