lundi 10 mars 2014

Modèle robuste: les relations dangereuses

Dans les articles précédents (JavaBean, la justification du mauvais orienté objet et Modèle robuste, immutabilité et Value Objects), j'ai montré comment créer un modèle robuste en laissant les objets assumer leurs responsabilités. Validation des paramètres, constructions atomiques, immutabilité ou value objects sont autant de patterns qui les aident à garantir leur cohérence.

Mais il est rare que les objets soient indépendants les uns des autres. Ils sont liés entre eux au travers de relations directes ou indirectes (via des collections), unidirectionnelles ou bidirectionnelles.

Un modèle est robuste si ses objets le sont, mais aussi si ses relations sont robustes. Dans l’ensemble, la gestion des relations n’est pas différente des autres propriétés. Après tout, sauf lorsqu’il s’agit de types primitifs, nous travaillons déjà avec des relations vers d’autres objets, même si nous nous sommes jusqu’à présent limités à des String ou d’autres Value Objects.

Relation simple


Le cas de base, c’est lorsqu’un objet A a une relation (une référence) vers un objet B. Dans ce cas, l’objet A doit veiller à la qualité de cette relation.

Par exemple, un objet Book a une relation vers un objet Category signifiant que le livre appartient à une catégorie. De plus, l'analyse du domaine montre que la catégorie DOIT être définie pour que l’objet livre soit cohérent.

Le code suivant est presque équivalent à celui de l’article précédent, sans nous encombrer du numéro Isbn. Pour simplifier cet exemple, le titre et l’auteur doivent être définis et immutables.
import org.apache.commons.lang3.StringUtils;

public class Book {
     private String title;
     private String author;
     private Category category;

     public Book(String title, String author, Category category) {
          if(StringUtils.isBlank(title) || StringUtils.isBlank(author)){
               throw new IllegalArgumentException("Ni le titre, ni l'auteur ne peut être null ou vide");
          }
          if(category == null){
               throw new IllegalArgumentException("La catégorie ne peut être null");
          }
          this.title = title.trim();
          this.author = author.trim();
          this.category = category;
     }

     public void setCategory(Category category) {
          if(category == null){
               throw new IllegalArgumentException("La catégorie ne peut être null");
          }
          this.category = category;
     }
     public Category getCategory() {
          return category;
     }
     public String getTitle() {
          return title;
     }
     public String getAuthor() {
          return author;
     }
}
Comme me l’a suggéré un collègue, je pourrais utiliser la méthode setCategory dans mon constructeur et éviter la duplication de la validation. Il faut toutefois être prudent: si j’héritais de la classe Book, je pourrais alors surcharger la méthode setCategory, la rendre par exemple moins restrictive, et construire des objets Book incohérents. Cela signifie que setCategory devrait alors être final.

Cette classe est robuste car elle garantit les conditions définies plus haut, y compris pour sa relation vers Category. Par contre, elle n’offre aucune garantie sur la qualité de la catégorie. Elle vérifie que null n’est pas passé en paramètre, mais pas que l’objet Category est cohérent.

C’est normal : chaque objet est responsable de sa propre cohérence. Ce n’est donc pas à Book de vérifier que l’objet Category est cohérent.
Cohérence et critères d'acceptation
Cela peut paraître un peu contradictoire, car pour le titre et l’auteur, nous vérifions que ces paramètres ne sont pas nuls et que les chaînes ne sont pas vides.

Cependant, nous ne vérifions pas la cohérence de la String : la String que nous recevons est cohérente de son point de vue. Une String contiendra toujours une chaîne de caractères, même s'il ne s'agit que d'espaces ("     ") ou de vide ("").

Il n’appartient pas à l’objet Book de vérifier la cohérence des String title ou author, mais il lui appartient de vérifier que leur contenu est acceptable pour lui.

Dans l’exemple ci-dessus, toute catégorie est acceptable puisque nous n’y avons mis aucune contrainte. Mais imaginons que nous ayons une classe ChildrenBook, un livre pour enfant. Nous ne voulons pas pouvoir lui attribuer une catégorie "gore" ou "érotique".

Pour remplir ce besoin, nous allons créer une propriété booléenne "adult" dans Category, à laquelle nous accèderons via un "isAdult". Quant à notre ChildrenBook, il étend désormais Book:
public class ChildrenBook extends Book{
     public ChildrenBook(String title, String author, Category category) {
          super(title, author, category);
          if(category.isAdult()){
               throw new IllegalArgumentException("pas de catégorie pour adulte dans un livre pour enfant");
          }
     }

     public final void setCategory(Category category) {
          if(category.isAdult()){
               throw new IllegalArgumentException("pas de catégorie pour adulte dans un livre pour enfant");
          }
          super.setCategory(category);
     }
}
A nouveau, ChildrenBook ne vérifie pas la cohérence de Category, mais vérifie que la catégorie passée en paramètre est acceptable pour lui.

Soit dit en passant, heureusement que setCategory n’était pas final… Cela nous permet de valider que notre catégorie est acceptable.

Relation multiple


Dans le cas d’une relation multiple, les choses sont un peu plus complexes. L’objet A référencie une collection d’objets B. Et le problème, c’est justement cette collection.

Etendons notre exemple de base: un livre appartient désormais à plusieurs catégories. Comme ci-dessus, nous nous contenterons de vérifier qu’aucune de ces catégories n’est nulle. On pourrait y ajouter un contrôle sur le contenu de catégorie, comme pour les livres pour enfants, le problème serait le même.
public class Book {
     private String title;
     private String author;
     private List<Category> categories;

     public Book(String title, String author, List<Category> categories) {
          if(StringUtils.isBlank(title) || StringUtils.isBlank(author)){
               throw new IllegalArgumentException("Ni le titre, ni l'auteur ne peut être null ou vide");
          }
          if(categories ==null || categories.isEmpty()){
               throw new IllegalArgumentException("La liste de catégories ne peut être nulle ou vide");
          }
          for(Category c:categories){
               if(c == null){
                    throw new IllegalArgumentException("Aucune élément de la liste de catégories ne peut être null");
               }
          }
          this.title = title.trim();
          this.author = author.trim();
          this.categories = categories;
     }

     public List<Category> getCategories() {
          return categories;
     }
     public String getTitle() {
          return title;
     }
     public String getAuthor() {
          return author;
     }
}
Cette implémentation semble respecter le contrat que nous nous étions fixé. Dans le cas présent, il n’y a pas de setter sur categories, ce qui fait qu’une fois la liste assignée, on ne peut plus en changer.

Attention, cela ne signifie pas que je ne peux pas ajouter de catégories ! Pire, cela ne signifie pas que je ne peux pas ajouter de catégorie null !

C’est même assez simple :
book.getCaterories().add(nouvelleCategorie) ; //Ajoute une catégorie aux catégories de book
book.getCategories().add(null); // Exactement ce qu’on ne voulait pas !
Le problème, c’est que le getCategories expose la collection de catégories, laquelle ne veille pas sur la qualité des éléments qui lui sont ajoutés. Normal, ce n’est pas son rôle.

Une solution consisterait à créer sa propre collection, en implémentant List ou en étendant ArrayList et en surchargeant la méthode add pour qu’elle vérifie les null.

Une autre possibilité, plus simple à mon avis, est de modifier l’implémentation de la méthode getCategories et, si le domaine permet l’ajout de catégories à un livre existant, d’implémenter la méthode addCategory de la manière suivante :
public void addCategory(Category category){
     if(category == null){
          throw new IllegalArgumentException("Une catégorie ne peut être nulle");
     }
     categories.add(category);
}

public List<Category> getCategories() {
     return Collections.unmodifiableList(categories);
}
La méthode addCategory vérifie la qualité des catégories ajoutées. Aucun null ne sera permis.

Quant à getCategories, elle renvoie maintenant une collection immutable contenant bien les catégories du livre. Plus question de l'utiliser pour modifier la collection, le développeur recevra une exception.

Parfait ? Non, il reste un problème important, illustré par le test (TestNg) suivant :
@Test
public void testAddNullCategoryAnyway(){
     Category sf = new Category("sf","Science-fiction");
     List<Category> cats = new ArrayList<Category>();
     cats.add(sf);

     Book b = new Book("Dune","Frank Herbert",cats);

     assertEquals(b.getCategories().size(), 1);

     cats.add(null); //Aie !

     assertEquals(b.getCategories().size(), 2, "Hé oui...");

     assertTrue(b.getCategories().contains(null), "Pire...");
}
Bien sûr, rappelez-vous, dans le constructeur :
this.categories = categories;
Donc, toute modification de la collection originale modifiera la liste des catégories du livre, sans vérifier la qualité de la modification.

La solution est simple, il suffit de remplacer la ligne précédente par celle-ci :
this.categories = new ArrayList<Category>(categories);
Hé oui ! Travailler avec des collections n’est pas simple.

Mais il y a pire : les relations bidirectionnelles.

Relation bidirectionnelle


Si un livre appartient à une ou plusieurs catégories et que chaque catégorie contient un ou plusieurs livres, la relation est dite bidirectionnelle. A noter que cela n’a rien à voir avec la cardinalité, juste avec le fait qu’un objet A possède une ou plusieurs références vers des objets B qui possèdent une ou plusieurs références vers l’objet A.

Le piège de ce type de relation, c’est qu’elle paraît toujours logique : un livre appartient à des catégories, et donc ces catégories contiennent le livre ; un chien a un maître, donc le maître a un chien; une facture a plusieurs lignes, donc chaque ligne appartient à une facture…

Les développeurs ont donc tendance à l’implémenter partout.

Mais logique ne veut pas dire intéressante. La question à se poser avant d’implémenter une relation bidirectionnelle, c’est de savoir si les deux navigations qu’elle propose (de A vers B ou de B vers A) sont aussi intéressantes l’une que l’autre. C’est l’analyse du domaine applicatif qui répondra à cette question.

Les relations bidirectionnelles sont surtout délicates à gérer. Outre les aspects déjà cités, il faut aussi tenir compte de la cohérence de la bidirectionnalité: si A a une référence vers B, alors B DOIT avoir une référence vers A.

Cela peut sembler superflu, mais je vois souvent des problèmes avec Hibernate qui sont simplement dus à une incohérence dans la relation bidirectionnelle.

Laissons Hibernate de côté et voyons comment gérer de manière robuste nos relations bidirectionnelles.

Gardons les principes de robustesse précédents et voyons le code pour Book et Category.
public class Category {
     private String name;
     private String description;
     private Set<Book> books;

     public Category(String name, String description) {
          if(StringUtils.isBlank(name)){
               throw new IllegalArgumentException("Le nom ne peut null ou vide");
          }
          this.name = name.trim();
          this.description = StringUtils.trimToNull(description);
          this.books = new HashSet<Book>();
     }

     public String getName() {
          return name;
     }
     public String getDescription() {
          return description;
     }
     public Set<Book> getBooks() {
          return Collections.unmodifiableSet(books);
     }

     public void addBook(Book book){
          if(book == null){
               throw new IllegalArgumentException("Le livre ne peut être null");
          }

          this.books.add(book);
          book.addCategory(this);
     }

     public int hashCode() {
          return name.hashCode();
     }

     public boolean equals(Object obj) {
          if (this == obj)
               return true;
          if (obj == null)
               return false;
          if (!(obj instanceof Category))
               return false;
          return name.equals(((Category) obj).name);
     }
}
public class Book {
     private String title;
     private String author;
     private Set<Category> categories;

     public Book(String title, String author) {
          if(StringUtils.isBlank(title) || StringUtils.isBlank(author)){
               throw new IllegalArgumentException("Ni le titre, ni l'auteur ne peut être null ou vide");
          }
          this.title = title.trim();
          this.author = author.trim();
          this.categories = new HashSet<Category>();
     }

     void addCategory(Category category){
          if(!category.getBooks().contains(this)){
               throw new IllegalArgumentException("La catégorie doit contenir le livre. Passez plutôt par category.addBook.");
          }
          categories.add(category);
     }

     public Set<Category> getCategories() {
          return Collections.unmodifiableSet(categories);
     }

     public String getTitle() {
          return title;
     }
     public String getAuthor() {
          return author;
     }

     public int hashCode() {
          return title.hashCode();
     }

     public boolean equals(Object obj) {
          if (this == obj)
               return true;
          if (obj == null)
               return false;
          if (!(obj instanceof Book))
               return false;

          return title.equals(((Book) obj).title);
     }
}
Que remarque-t-on ?
  • La collection de catégories n’est plus une List, mais un Set (idem pour les livres dans les catégories). Ce n’est pas absolument nécessaire et ça aurait pu être fait auparavant. Ca permet d’éviter facilement les doublons (ajouter deux fois le même livre à une catégorie) à condition d’implémenter correctement les méthodes equals/hashcode. Dans le cas présent, Book n’a plus vraiment d’attribut adéquat (Isbn en était un) et le equals a été fait sur le titre, ce qui n’est pas correct dans le monde réel. Au niveau Category, on peut supposer que le nom d’une catégorie est unique. Une petite remarque au passage : je trouve que les développeurs utilisent plus facilement des List que des Set, et c’est dommage. Car imaginez ce code avec des List : lourd…
  • Le code part du principe qu’un livre est ajouté à une catégorie et non l’inverse (on aurait facilement pu travailler dans l’autre sens). Il n’est donc normalement pas possible (c’est-à-dire pas des conditions normales) d’ajouter une catégorie à un livre.
  • La méthode addCategory existe pourtant bien dans Book, mais sa visibilité est réduite au package. C’est un pis-aller (la visibilité devrait être réduite à la classe Category, ce que Java ne permet pas de faire). Elle est destinée à n'être appelée que par Category pour garantir la bidirectionnalité. Dans le cas où elle serait malgré tout appelée (depuis une autre classe appartenant au même package), elle vérifie que la Category contient bien le livre, sans quoi elle lance une exception conseillant de passer par addBook de Category. Elle ne vérifie plus que le paramètre n’est pas null (puisqu’il ne peut plus l’être).
  • La méthode addBook de Category ajoute le livre et appelle addCategory de Book pour garantir la bidirectionnalité.
  • Il est encore plus important que jamais d’interdire l’ajout direct d’un objet dans les collections books et categories en passant par getBooks ou getCategories() !
A part cette visibilité "package", ce code est plutôt robuste et assez simple à développer. Il existe d’autres options qui éliminent le risque précécédent.
Permettre le addBook et le addCategory
Plutôt que de prendre le risque d’appeler de manière illégale la méthode addCategory de Book, bien que cet appel garantisse lui-même la cohérence de la relation, une autre approche consiste à autoriser aussi bien addBook que addCategory.

La méthode addBokk devient :
public void addBook(Book book){
     if(book == null){
          throw new IllegalArgumentException("Le livre ne peut être null");
     }

     this.books.add(book);
     if(! book.getCategories().contains(this)){
          book.addCategory(this);
     }
}
La method addCategory, maintenant publique, devient:
public void addCategory(Category category){
     if(category == null){
          throw new IllegalArgumentException("La catégorie ne peut être nulle");
     }
     categories.add(category);
     if(! category.getBooks().contains(this)){
          category.addBook(this);
     }
}
Afin d’éviter une boucle sans fin, il faut vérifier si l’ajout a déjà été fait de l’autre côté et, si ce n'est pas le cas, établir la bidirectionnalité.
Réflexion
Une dernière approche consiste à éliminer le risque d’appel à addCategory, en mettant la méthode en private. La méthode addBook de Category devient alors :
public void addBook(Book book){
     if(book == null){
          throw new IllegalArgumentException("Le livre ne peut être null");
     }

     try {
          Method addCategoryMethod = Book.class.getDeclaredMethod("addCategory", Category.class);
          addCategoryMethod.setAccessible(true);
          addCategoryMethod.invoke(book, this);
          this.books.add(book);
     } catch (SecurityException e) {
          throw new RuntimeException("TODO...",e);
     } catch (NoSuchMethodException e) {
          throw new RuntimeException("TODO...",e);
     } catch (IllegalArgumentException e) {
          throw new RuntimeException("TODO...",e);
     } catch (IllegalAccessException e) {
          throw new RuntimeException("TODO...",e);
     } catch (InvocationTargetException e) {
          throw new RuntimeException("TODO...",e);
     }
}
Je pense que c’est un des rares cas où la réflexion est tout à fait acceptable dans le cadre du développement du modèle.

Certaines optimisations sont possibles, comme charger l’objet addCategoryMethod dans le constructeur de la classe Category, voire dans une initialisation statique.

Certains pourraient être tentés de ne pas implémenter de méthode addCategory dans Book et de modifier directement, par réflexion, la collection de Category de Book.

C’est bien sûr possible, mais je le déconseille. Même si l’on triche un peu en utilisant la réflexion, on laisse le soin au livre de veiller à la cohérence de ses attributs.

Et le modèle est robuste.

Aucun commentaire:

Enregistrer un commentaire