Affichage des articles dont le libellé est qualité. Afficher tous les articles
Affichage des articles dont le libellé est qualité. Afficher tous les articles

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.

samedi 22 février 2014

Modèle robuste: immutabilité et Value Objects

Dans l'article JavaBean, la justification du mauvais orienté objet, nous avons vu comment utiliser les constructeurs et les setters pour garantir que les valeurs passées aux objets étaient correctes, en respectant le principe essentiel que l’objet était le seul responsable de la qualité de ses données.

Cette responsabilité peut être partagée par les paramètres eux-mêmes et c’est ce que nous allons voir dans un instant.

A toute épreuve?


Mais revenons un instant sur la dernière version de l’objet Rectangle de l’article précédent. Elle est parfaitement robuste et ne permet pas la création d’un objet incohérent.

Vraiment ?

Oui, vraiment, dans des conditions normales d’utilisation.

Là... vous le voyez, le piège? Des conditions normales d’utilisation, c’est-à-dire lorsque le développeur n’utilise que l’interface que l’objet lui offre.

S'il utilise la réflexion pour accéder directement aux attributs de l’objet, rien ne peut malheureusement garantir sa cohérence, comme le montre le test (TestNg) suivant :
@Test
public void testCanCreateIncorrectRectangleByReflection(){
     Rectangle r = new Rectangle(2.0,1.0);
     try {
          Field dimension1 = Rectangle.class.getDeclaredField("dimension1");
          Field dimension2 = Rectangle.class.getDeclaredField("dimension2");
          dimension1.setAccessible(true);
          dimension2.setAccessible(true);
          dimension1.set(r, -6.0);
          dimension2.set(r, 0.0);
     } catch (SecurityException e) {
          fail("Security manager...");
     } catch (NoSuchFieldException e) {
          fail("Etonnant...");
     } catch (IllegalArgumentException e) {
          fail("Etonnant...");
     } catch (IllegalAccessException e) {
          fail("Etonnant...");
     }

     assertEquals(r.getLongueur(),0.0);
     assertEquals(r.getLargeur(),-6.0);
     assertEquals(Math.abs(r.getSurface()),0.0);
     assertEquals(r.getPerimetre(),-12.0);
}
Ces quelques lignes permettent d’avoir un rectangle avec une dimension nulle et une autre négative, et des propriétés incohérentes.

Il n’y a malheureusement rien à faire pour contrer cela. Cependant, la réflexion reste lourde à utiliser (raison pour laquelle je suis généralement contre le développement de méthodes utilitaires qui faciliteraient la réflexion). Ceux qui l’utilisent le font, on peut le supposer, en connaissance de cause et assument les risques que cela entraîne.

Immutabilité


Poursuivons notre étude des modèles robustes et examinons une notion abordée succinctement dans l’article précédent : l’immutabilité. (A noter qu’en français, immutabilité n’existe pas et que c’est immuabilité qu’il faut utiliser. Néanmoins, je continuerai d’utiliser immutabilité, proche du mot anglais d’origine « immutability ».)

L’immutabilité signifie que, une fois l’objet créé, ses propriétés ne peuvent plus être modifiées.

Ceci pourrait soulever débat : sont-ce les attributs ou les propriétés qui ne peuvent plus être modifiés. Dans l’article précédent, j’ai fait une distinction entre les attributs (champs définis dans la classe) et les propriétés (valeurs exposées par la classe, de manière standard via des setters et des getters). Un objet peut utiliser certains attributs, mais exposer de propriétés différentes.

Dès lors, l’immutabilité touche-t-elle les attributs ou les propriétés ? Si les attributs sont immutables, quel effet pourrait-on attendre de la modification des propriétés ? Aucun. Par contre, les propriétés pourraient être immutables, mais les attributs seraient modifiables par l’objet lui-même, étant donné qu’il est le seul responsable de ses attributs.

Pour couper court à la discussion, je considérerai qu’attributs et propriétés sont immutables.

L’immutabilité peut aussi toucher à certaines propriétés et non à toutes. Dans ce cas, l’objet lui-même n’est bien sûr pas immutable.

C’est l’analyse du domaine de l’application qui déterminera ce qui est immutable ou non.

Pour ce qui suit, je vais utiliser une nouvelle classe : Book. Il s’agit d’une classe qui définit un livre (normalement, il n’était pas nécessaire de passer par Google Traduction pour le comprendre) lequel a les propriétés suivantes :
  • un titre
  • un auteur
  • un numéro ISBN
L'ISBN est un numéro de codification unique, par édition. Il doit respecter certaines règles pour être valide.

Pour ce qui suit, supposons que les critères de validité des propriétés soient les suivants :
  • Le numéro ISBN doit toujours être défini et doit être valide. Une fois défini, il n’y a pas moyen de le modifier.
  • Le titre doit toujours être défini par une chaîne non vide. Une fois défini, il ne peut être modifié.
  • L’auteur est facultatif. Il peut être modifié. S’il est défini, il doit être une chaîne non vide.
Notons également que le numéro ISBN constitue une clé métier.
Pour créer cette classe, je vais de plus employer deux librairies utilitaires:
La classe Book répondant aux conditions ci-dessus est la suivante :
public class Book {
     private String isbn;
     private String title;
     private String author;

     public Book(String isbn, String title){
          this(isbn,title,null);
     }

     public Book(String isbn, String title, String author){
          ISBNValidator validator = ISBNValidator.getInstance(true);
          if(! validator.isValid(isbn)){
               throw new IllegalArgumentException("Isbn incorrect");
          }
          if(StringUtils.isBlank(title)){
               throw new IllegalArgumentException("Le titre ne peut être vide");
          }
          this.isbn = validator.validate(isbn);
          this.title = title.trim();
          this.author = StringUtils.trimToNull(author);
     }

     public void setAuthor(String author) {
          this.author = StringUtils.trimToNull(author);
     }

     public String getIsbn() {
          return isbn;
     }

     public String getTitle() {
          return title;
     }

     public String getAuthor() {
          return author;
     }

     @Override
     public int hashCode() {
          return isbn.hashCode();
     }

     @Override
     public boolean equals(Object obj) {
          if (this == obj)
               return true;
          if (obj == null)
               return false;
          if (!(obj instanceof Book))
               return false;
          Book other = (Book) obj;
          return isbn.equals(other.isbn);
     }
}
Quelques remarques :

  • L’IsbnValidator valide les ISBN-13 et ISBN-10, la valeur null étant non valide. Il va aussi les convertir (enlever les ‘-‘) et transformer les ISBN-10 en ISBN-13. Le résultat est une String de 13 chiffres.
  • Le getIsbn renvoie la String non formatée. On pourrait aussi la formater d’une manière standard (xxx-x-xxxx-xxxx-x).
  • Un "trim" est appliqué au titre et à l’auteur afin de ne pas garder des espaces inutiles à l'avant et à l'arrière.
  • Si l’auteur est vide ("      ", par exemple), la valeur de l’attribut sera null.
  • Il y a deux constructeurs, un avec un auteur, l’autre sans. A noter que l’auteur peut être null dans le premier.
  • La méthode equals porte sur le numéro ISBN  uniquement puisqu’il correspond à une clé métier (trop souvent, je vois des equals sur l’ensemble des attributs).
  • Il n'y a qu'un seul setter, sur l'auteur, lequel valide le paramètre. C'est la seule propriété mutable.
  • Si vous demandez à Eclipse de générer le code du equals/hashcode à partir de l’attribut isbn, vous obtiendrez le code suivant :
    @Override
    public int hashCode() {
         final int prime = 31;
         int result = 1;
         result = prime * result + ((isbn == null) ? 0 : isbn.hashCode());
         return result;
    }
    
    @Override
    public boolean equals(Object obj) {
         if (this == obj)
              return true;
         if (obj == null)
              return false;
         if (!(obj instanceof Book))
              return false;
         Book other = (Book) obj;
         if (isbn == null) {
              if (other.isbn != null)
                   return false;
         } else if (!isbn.equals(other.isbn))
              return false;
         return true;
    }
    
    Néanmoins, notre modèle garantissant que l'attribut isbn n’est jamais null, certains contrôles sont superflux.
Notre classe Book est robuste. Elle ne peut contenir aucune valeur incorrecte. Les propriétés non mutables ne peuvent être modifiées. C’est parfait… enfin presque.

Value Object


C’est la question piège : "si vous deviez avoir un attribut qui est un numéro de compte/un isbn/un numéro de sécurité sociale… quel type utiliseriez-vous ?". Piège, car trop souvent la réponse va être "une String" (même si "un long" aurait été pire). Une String, c’est facile d’emploi et ça peut contenir n’importe quoi… vraiment n’importe quoi.

C’est le cas de notre isbn qui est une String. Nous sommes obligés de le valider lorsqu’on le passe en paramètre du constructeur. Tant qu’on reste dans le contexte de Book, ce n’est pas vraiment un problème. Mais si isbn devient un paramètre de méthode, il faudra peut-être le valider à chaque utilisation.

L’erreur est plus fondamentale : un des atouts de Java est d’avoir un typage fort. Lorsque j’utilise un Integer, je suis sûr que c’est un nombre entier, pas un décimal, pas une chaîne de caractères. Par contre, une String pour un numéro ISBN, c’est particulièrement faible, car cette String peut contenir n'importe quoi avant d'être validée.

La solution, ce sont les Value Objects.

Littéralement, ce sont des objets qui contiennent une valeur (sans entrer dans les détails, cela ne signifie pas nécessairement qu’ils n’ont qu’un seul attribut ou une seule propriété).

Integer est un exemple de Value Object, un objet qui contient un entier comme valeur. String aussi (objet qui contient une chaîne de caractères, quoi qu’elle représente).

Le numéro ISBN pourrait être avantageusement remplacé par un Value Object.

Avant de montrer le code, voyons quelques propriétés des Value Objects :
  • La valeur contenue dans le Value Object est toujours correcte (cohérence de l’objet). Essayez new Integer("toto"), vous m’en direz des nouvelles.
  • Un Value Object est généralement immutable. String, Integer, BigDecimal sont tous immutables (c’est même une erreur commune de croire le contraire).
  • On pourrait s’attendre à ce que deux Value Objects contenant la même valeur soient en fait la même référence. C’est rarement le cas, car c’est très difficile à implémenter (sauf si le nombre de valeurs différentes est petit).
Tenant compte de ces remarques, voici une classe pour les Value Objects ISBN :
public class Isbn {
     private String isbn;

     public Isbn(String isbn){
          if(!validate(isbn)){
               throw new IllegalArgumentException("Isbn incorrect");
          }
          this.isbn = ISBNValidator.getInstance(true).validate(isbn);
     }

     public Isbn(Isbn isbn){
          this.isbn = isbn.isbn;
     }

     public static boolean validate(String isbn){
          return ISBNValidator.getInstance(true).isValid(isbn);
     }

     public String getIsbn() {
          return isbn;
     }

     @Override
     public int hashCode() {
          return isbn.hashCode();
     }

     @Override
     public boolean equals(Object obj) {
          if (this == obj)
               return true;
          if (obj == null)
               return false;
          if (!(obj instanceof Isbn))
               return false;
          Isbn other = (Isbn) obj;

          return isbn.equals(other.isbn);
     }
}
Quelques points notables :
  • L’objet reste construit à partir d’une String. Mais cette String n’est utilisée qu’une seule fois, comme paramètre du construteur. Elle peut être une valeur entrée par l’utilisateur. Néanmoins, c’est le seul endroit où une String représentera un numéro ISBN. Une fois l’objet Isbn construit, plus aucune méthode une String pour un ISBN. Ceci dit, au sein de l'objet Isbn, le numéro est encodé dans une String.
  • Le deuxième constructeur n’est pas absolument nécessaire. C’est un constructeur de copie, parfois utile.
  • La méthode validate, statique, permet de valider la chaîne de caractère supposée contenir un numéro ISBN. Elle évite de devoir faire un try-catch autour de la construction du Value Object. Au lieu de :
    try {
         isbn = new Isbn(une string);
    } catch(IllegalArgumentException e) {
         //Gérer le fait que la String n'est pas valide
    }
    
    on écrira :
    if(Isbn.validate(une string)){
         isbn = new Isbn(une string);
    } else {
         //Gérer le fait que la String n'est pas valide
    }
    
    Bien sûr, la validation sera faite deux fois, mais l’écriture est plus propre.
  • Pour la méthode equals notez que l’attribut isbn n’est jamais null.
  • Une partie de la logique que l’on trouvait dans Book est maintenant dans Isbn.
Le code de Book devient :
public class Book {
     private Isbn isbn;
     private String title;
     private String author;

     public Book(Isbn isbn, String title){
          this(isbn,title,null);
     }

     public Book(Isbn isbn, String title, String author){
          if(isbn==null){
               throw new IllegalArgumentException("Isbn doit être fourni");
          }
          if(StringUtils.isBlank(title)){
               throw new IllegalArgumentException("Le titre ne peut être vide");
          }
          this.isbn = isbn;
          this.title = title.trim();
          this.author = StringUtils.trimToNull(author);
     }

     public void setAuthor(String author) {
          this.author = StringUtils.trimToNull(author);
     }

     public Isbn getIsbn() {
          return isbn;
     }

     public String getTitle() {
          return title;
     }

     public String getAuthor() {
          return author;
     }

     @Override
     public int hashCode() {
          return isbn.hashCode();
     }

     @Override
     public boolean equals(Object obj) {
          if (this == obj)
               return true;
          if (obj == null)
               return false;
          if (!(obj instanceof Book))
               return false;
          Book other = (Book) obj;
          return isbn.equals(other.isbn);
     }
}
Le résultat n’est pas beaucoup plus court qu’auparavant, mais à présent l’objet Isbn s’occupe de sa propre cohérence.

Malheureusement, nous devons toujours vérifier que la référence passée en paramètre pour isbn n’est pas nulle.

Jusqu’où aller ?


Ne serait-il pas intéressant d’utiliser un Value Object pour le titre, ce qui garantirait qu’il y a un contenu ? Dans le premier exemple de JavaBean, la justification du mauvais orienté objet, dans les personnes à charge, ne serait-il pas intéressant d’avoir un StrictlyPositiveInteger qui encapsulerait un entier obligatoirement strictement positif ?

Il n’y a pas de réponse catégorique à ces questions. Le point important à considérer est la réutilisation du Value Object : si la classe est peu utilisée hors d’un contexte spécifique (c'est-à-dire, un autre objet), un Value Object a peu d’intérêt.

Malgré tout, on pourrait être tenté d’utiliser des Value Object partout, mais cela demande du travail supplémentaire. D’autant plus que, comme nous le verrons une prochaine fois, il faudra mettre la main à la pâte pour faire fonctionner ces objets avec Hibernate.

vendredi 14 février 2014

JavaBean: la justification du mauvais orienté objet

Il y a quelques semaines, je découvrais le code suivant (à noter que les extraits de code présentés ici ne sont pas les codes réels, cependant les principes restent les mêmes) :
public class Situation {
     private int enfantsACharge;
     private int autresACharge;
     private boolean personneACharge;

     public int getEnfantsACharge() {
          return enfantsACharge;
     }
     public void setEnfantsACharge(int enfantsACharge) {
          this.enfantsACharge = enfantsACharge;
     }
     public int getAutresACharge() {
          return autresACharge;
     }
     public void setAutresACharge(int autresACharge) {
          this.autresACharge = autresACharge;
     }
     public boolean isPersonneACharge() {
          return personneACharge;
     }
     public void setPersonneACharge(boolean personneACharge) {
          this.personneACharge = personneACharge;
     }
}

Cette classe décrit la situation fiscale d’une personne, à savoir le nombre d’enfants à charge, le nombre d’autres personnes à charge et si elle a ou non des personnes à charge. Rien ne vous choque ?

Non? Continuons…

Un peu plus loin, un service :
public class SituationService {
     public Situation createSituation(int enfantsACharge, int autresACharge){
          Situation situation = new Situation();
          situation.setEnfantsACharge(enfantsACharge);
          situation.setAutresACharge(autresACharge);
          situation.setPersonneACharge(enfantsACharge !=0 || autresACharge != 0);
          return situation;
     }
}
Toujours rien ne vous choque ?

Si vous répondez une nouvelle fois non, c’est que les JavaBeans ont eu raison de vous et que le domaine anémique (Anemic Domain Model) est votre quotidien. Vous ne faites donc plus de l’orienté objet, mais un ersatz procédural dans lequel les objets sont, soit des conteneurs de données, soit des conteneurs de méthodes. Bref, de la mauvaise programmation orientée objet.

Voici une version plus correcte et plus orientée objet de la situation fiscale :
public class Situation {
     private int enfantsACharge;
     private int autresACharge;

     public void setEnfantsACharge(int enfantsACharge) {
          if(enfantsACharge < 0){
               throw new IllegalArgumentException("Le nombre d'enfants à charge doit être positif");
          }
          this.enfantsACharge = enfantsACharge;
     }
     public int getEnfantsACharge() {
          return enfantsACharge;
     }
     public void setAutresACharge(int autresACharge) {
          if(autresACharge < 0){
               throw new IllegalArgumentException("Le nombre d'autres personnes à charge doit être positif");
          }
          this.autresACharge = autresACharge;
     }
     public int getAutresACharge() {
          return autresACharge;
     }
     public boolean isPersonnesACharge(){
          return enfantsACharge > 0 && autresACharge > 0;
     }
}
Quant à la méthode createSituation de SituationService, elle disparaît du service (lequel a peut-être d'autres raisons, légitimes, d'exister).

Cette version apporte quelques améliorations :

  • Il est impossible de passer un nombre négatif pour les enfants ou les autres à charge. Les setters vérifient la qualité des paramètres et ne les acceptent que s’ils sont corrects.
  • Le statut "personnesACharge" est déterminé par l’objet lui-même. Avant, sa valeur dépendait d’un calcul extérieur et rien ne garantissait qu’il soit calculé correctement. Accessoirement, l’attribut "personnesACharge" a disparu. Il reste juste un accesseur (qui suit la standard JavaBean, hé oui…).
Au final, ce modèle est plus robuste que le premier. Il n’y a plus moyen de créer un objet Situation qui n’aurait aucune cohérence (3 enfants à charge, mais personnesACharge false). Et qu’on ne vienne pas me dire que le service permettait d'en faire autant, puisque l’objet pouvait être créé à l'extérieur du service.

La première fois que j’ai présenté cette approche à un collègue, il s’est écrié : « si tu fais ça, tu vas casser le contrat JavaBean ».

A cela, je réponds deux choses :
  1. "oui, et alors ?" parce que les JavaBeans ne sont pas une religion (encore moins un contrat) et que les frameworks qui prétendent en avoir besoin (Hibernate, Spring) mentent un peu (je verrai ça une autre fois).
  2. "de toute façon, la classe Situation du départ n’était pas non plus un JavaBean" : elle n’implémente pas Serializable (exercice: comptez le nombre de vos entités Hibernate/Jpa, soit disant des JavaBeans, qui implémentent Serializable).

Modèle robuste

La propriété la plus importante de l’orienté objet, c’est l’encapsulation : l’objet est le seul responsable de la qualité de ses attributs et décide seul des informations et des opérations qu’il veut exposer vers l’extérieur.

Pour continuer ma démonstration, je vais utiliser une classe plus simple, mais mal écrite : un rectangle.
public class Rectangle {
     private double longueur;
     private double largeur;

     public double getLongueur() {
          return longueur;
     }
     public void setLongueur(double longueur) {
          this.longueur = longueur;
     }
     public double getLargeur() {
          return largeur;
     }
     public void setLargeur(double largeur) {
          this.largeur = largeur;
     }
}
Soyons sérieux ! Ce n’est pas de l’orienté objet. Pourtant, nombreux sont ceux qui s'imaginent respecter l'encapsulation parce que les attributs sont private...

Récemment, un développeur me disait, un peu dépité, "je ne vois pas pourquoi on continue de mettre les attributs en privé si de toute façon les setters et les getters permettent de les modifier directement". Il avait raison !

Dans le cas présent, un objet Rectangle n’a aucun contrôle sur la qualité des propriétés qui lui sont injectées.
Rectangle r = new Rectangle() ;
r.setLongueur(-3.0) ; //Non, mais allô quoi...
C’est absurde. Sans pousser trop l’analyse du domaine, il est clair qu’un rectangle ne peut avoir une longueur ou une largeur négative, ni même égale à 0.

Là où je suis le plus étonné, c'est que de nombreux développeurs pensent que la spécification JavaBean leur impose d’avoir des setters ridicules.

Première étape

Nous commencerons donc par consolider notre modèle du domaine en réécrivant les setters de manière intelligente :
public class Rectangle {
       private double longueur;
       private double largeur;
       public double getLongueur() {
             return longueur;
       }
       public void setLongueur(double longueur) {
             if(longueur <= 0){
                    throw new IllegalArgumentException("La longueur doit être strictement positive");
             }
             this.longueur = longueur;
       }
       public double getLargeur() {
             return largeur;
       }
       public void setLargeur(double largeur) {
             if(largeur <= 0){
                    throw new IllegalArgumentException("La largeur doit être strictement positive");
             }
             this.largeur = largeur;
       }
}
Impossible à présent d'injecter des valeurs incorrectes dans notre objet, les setters y veillent.

Le choix de l'exception est logique. C'est une RuntimeException pour laquelle on ne fait normalement aucun try-catch. Si cette erreur arrive, c'est que le développeur n'a pas été vigilant: avant d'utiliser les paramètres, il devait les valider. Lorsqu'elle arrive, il n'y a rien qu'on puisse faire. Il fallait agir avant. De la même manière qu'on ne fait pas de try-catch pour des NullPointerException, mais qu'on vérifie les références suspectes en les comparant à null.

Cette classe ne permet pas d'injecter dans ses instances des valeurs incorrectes.

En fait, ce n'est pas tout à fait exact puisqu'une classe qui hériterait de Rectangle pourrait surcharger les setters et les réécrire sans la validation. La réplique à ce risque consiste à mettre les setters en final.

En dehors de ce risque, peut-on considérer qu'une instance de Rectangle sera désormais correcte?

Deuxième étape

La réponse à la question précédente est hélas! non, comme le démontre le code suivant:
Rectangle r = new Rectangle();
r.setLongueur(3.0);
System.out.println(r.getLargeur());
Réponse... 0 ! Soit une largeur inacceptable. L'objet n'est pas cohérent.

Moralité, si certaines propriétés doivent obligatoirement être remplies, la création de l'objet doit être atomique.

Le moyen le plus simple pour obtenir cette atomicité est de passer par un constructeur.
public class Rectangle {
       private double longueur;
       private double largeur;
       
       public Rectangle(double longueur, double largeur){
             if(longueur <= 0){
                    throw new IllegalArgumentException("La longueur doit être strictement positive");
             }
             if(largeur <= 0){
                    throw new IllegalArgumentException("La largeur doit être strictement positive");
             }
             this.largeur = largeur;
             this.longueur = longueur;
       }
       public double getLongueur() {
             return longueur;
       }
       public double getLargeur() {
             return largeur;
       }
       public double getSurface(){
             return longueur * largeur;
       }
}
Là, plus moyen de se tromper. Lorsqu'il est créé, un rectangle a une longueur et une largeur, toutes deux strictement positives.

J'ai aussi introduit un autre aspect: l'objet est immutable. Une fois le rectangle créé avec une longueur et une largeur données, aucune de ses deux dimensions ne peut être modifiées. C'est le domaine de l'application qui déterminera si l'immutabilité est une propriété de l'objet ou non. Il est également possible que seules certaines propriétés soient immutables et pas les autres. D'une manière générale, je trouve que les développeurs ne font pas assez attention à l'immutabilité et que leur modèle gagnerait en qualité si c'était le cas.

Si le Rectangle devait avec une longueur mutable, il suffirait d'y ajouter une méthode setLongueur, qui vérifie la qualité du paramètre et qui permet de modifier l'attribut longueur précédemment fixé dans le constructeur.

Parfait? Presque, mais on peut mieux faire...

Troisième étape

Vous allez dire que je chicane (et c'est certainement vrai), mais rien ne m'empêche de construire un rectangle où la longueur est plus courte que la largeur.

Allez, un petit dernier:
public class Rectangle {
       private double dimension1;
       private double dimension2;
       
       public Rectangle(double dimension1, double dimension2){
             if(dimension1 <= 0 || dimension2 <= 0){
                    throw new IllegalArgumentException("Les dimensions doivent être strictement positives");
             }
             this.dimension2 = dimension1;
             this.dimension1 = dimension2;
       }
       public double getLongeur() {
             return dimension1>dimension2?dimension1:dimension2;
       }
       public double getLargeur() {
             return dimension1>dimension2?dimension2:dimension1;
       }
       public double getSurface(){
             return dimension1 * dimension2;
       }
}
Maintenant, la longueur est plus grande que la largeur. Ce qui est intéressant avec cette manière de procéder, c'est qu'elle montre bien le principe d'encapsulation. L'objet expose certaines propriétés (avec des getters, puisque c'est le standard JavaBean): sa longueur, sa largeur, sa surface et je pourrais y ajouter son périmètre. Mais ces propriétés ne correspondent en fait à aucun attribut de l'objet, lequel fait ce qu'il veut tant qu'il présente une interface cohérente.

Il y a moyen d'améliorer cette classe: on peut lui ajouter un equals (et ceux qui croient qu'il suffit de vérifier l'égalité des propriétés se planteront), un hashcode, un toString...

Un détail peut éventuellement déranger (en particulier si comme moi vous avez fait du C++ où c'est considéré comme une erreur): le constructeur lance une exception. Ce n'est pas un problème en Java.

Factory et builder

Il n'y a pas besoin d'améliorer la robustesse de ce Rectangle. La construction à l'aide des constructeurs est suffisante. Mais il existe deux autres patterns de construction qui permettent éventuellement de renforcer le modèle: la factory et le builder.

Une factory est utile pour créer une instance d'une des nombreuses implémentations d'une classe, que l'implémentation est choisie en fonction des paramètres de création et que l'on souhaite masquer l'implémentation réellement utilisé. Un builder est quant à lui intéressant lorsqu'on est amené à créer de nombreux constructeurs pour un objet, parce qu'il y a diverses combinaisons de propriétés possibles, ou que plusieurs paramètres peuvent être ignorés ou nulls. Le builder permet aussi d'avoir une interface "fluent" pour la création d'un objet.

Attention cependant qu'aucun des deux patterns ne doit prendre la place de la solidité du modèle. Ce n'est pas parce qu'une factory garantit la création atomique d'un objet cohérent qu'il doit être possible de créer un objet incohérent en se passant de la factory (c'est le problème posé par le service au début). Le modèle doit être solide par lui même.

Ni l'un ni l'autre n'a beaucoup de sens ici. Je pourrais éventuellement créer un version euclidienne du rectangle et une version... non-euclidienne, mais je me vais me contenter de pousser le raisonnement de la factory dans un design que je trouve finalement assez élégant.

Comme je veux masquer l'implémentation, je réduis le rectangle à une interface qui expose ses propriétés via de getters.
public interface Rectangle {
       double getLongeur();
       double getLargeur();
       double getSurface();
       double getPerimetre();
}
Et je laisse la factory créer l'implémentation à l'aide d'une inner classe anonyme...
public class RectangleFactory {
       public static Rectangle create(final double dimension1,final double dimension2){
             if(dimension1 <= 0 || dimension2 <= 0){
                    throw new IllegalArgumentException("Les dimensions doivent être strictement positives");
             }

             Rectangle r = new Rectangle(){
                    private double longueur = dimension1>dimension2?dimension1:dimension2;
                    private double largeur = dimension1>dimension2?dimension2:dimension1;
                    
                    public double getLongeur() {
                           return longueur;
                    }

                    public double getLargeur() {
                           return largeur;
                    }

                    public double getSurface() {
                           return longueur*largeur;
                    }

                    public double getPerimetre() {
                           return 2*(longueur+largeur);
                    }
                    
             };
             
             return r;
       }
}
Encore une fois, le rectangle obtenu est robuste: longueur et largeur strictement positives, longueur plus grande que la largeur.

Tiré par les cheveux? Certes, mais la Factory n'aurait pas eu d'intérêt si j'avais pu créer le rectangle par d'autres moyens. En en faisant une inner class, c'est presque totalement impossible (rien n'empêche en fait un développeur de créer une implémentation non robuste de mon interface...).

Ce qu'il faut en retenir

La spécification JavaBean a été écrite pour faciliter l'utilisation de langages script (comme dans les jsp). Si "r" représente une instance de Rectangle, alors en langage scripté "r.longueur" sortira sa longueur. Grâce à la spécification, le langage scripté sait qu'il doit trouver l'information, non pas dans un attribut longueur (qui n'existe peut-être pas), mais grâce à une méthode getLongueur qui renvoie la valeur d'une propriété, laquelle peut venir directement d'un attribut ou être calculée.

Rien dans la spécification JavaBean n'oblige :
  • à avoir systématiquement un setter et un getter pour chaque attribut
  • que les setters ou les getters correspondent à des attributs réels
  • que les setters ou les getters soient écrits de la manière la plus basique (stupide?) possible
  • qu'il n'y ait aucun constructeur avec paramètres (par contre il en faut obligatoirement un sans paramètre, ce qui n'est pas le cas ici)
  • à n'avoir aucune autre méthode que les setters et les getters (si, si... j'ai déjà entendu ça comme justification de l'absence d'une méthode equals)
Dans de prochains articles, je vous montrerai d'autres moyens de construire un modèle robuste, mais aussi qu'un framework comme Hibernate, pour lequel il est "bien connu" que les entités doivent être des JavaBeans, n'en a pas du tout besoin et fonctionne très bien avec un modèle robuste (et quelques points d'attention).

Les JavaBeans ne sont pas le mal absolu et ils ont leur utilité. Mais comme pour toute chose en programmation, il est important de comprendre ce que l'on fait et pourquoi.

L'automatisme dans la création d'une classe qui consiste à écrire les attributs en private et demander à Eclipse de générer automatiquement les getters et setters est une absurdité.

On peut rarement transiger avec la qualité du modèle.

vendredi 18 février 2011

"Si ça marche, ce n'est pas une erreur"

Un des points qui me tiennent à coeur, c'est la qualité des développements. Et une grande partie de ma fonction consiste justement à veiller à cette qualité. La question toutefois est de savoir ce qu'est la qualité d'une application.

Un incident récent suggère que pour certains, il suffit que l'application fasse ce qu'on lui demande. Je suis d'accord que c'est un point essentiel, mais il ne suffit cependant pas à attribuer un label de qualité à une application.

La preuve avec un exemple bien réel.

L'analyse quotidienne faite par Sonar révèle un problème potentiel sur une application. Voici le code :
Integer i = Integer.valueOf(0);
Integer j = Integer.valueOf(1)
//Opérations sur i et j
if(i == j){
    //la suite du code
Le problème, c'est le "==" entre deux objets. Sonar soupçonne, avec raison, que c'est l'égalité des objets qui est testée et non l'égalité des références. Le code correct est donc:
if(i.equals(j))
C'est la correction la plus directe. Néanmoins, dans le cas présent, une meilleur approche est de transformer les Integer en int car ils ne se justifient pas et entraînent une perte de performance.

L'erreur est signifiée au développeur qui hausse les épaules et répond: "pourtant, ça marche".

Le fait est que la classe Integer, pour des valeurs comprises entre -128 et 127 utilise un cache, à condition de passer par valueOf (ce qui est le cas ici, mais aussi des opérations de boxing/unboxing).
Dans ces conditions, plusieurs Integer encapsulant le même "int" compris entre ces valeurs auront la même référence. Or, dans les opérations effectuées ci-dessus avec i et j, leurs valeurs sont comprises entre 0 et environ 10. Conséquence: bien qu'incorrect d'un point de vue OO, le test fonctionne.

Le développeur se tourne vers son chef d'équipe qui rétorque: "Si ça fonctionne, je ne vois pas où est le problème. De toute façon, on n'a pas le temps (sic)."

Au-delà de l'incident, on est sans doute tenté de se poser la question "pourquoi modifier ce code puisque ça marche?".

Voici quelques réponses:
  1. parce que si les valeurs des Integer dépassent les limites du cache, le code ne fonctionnera plus
  2. parce que si les valueOf sont remplacés par des new Integer(), ça ne fonctionnera plus
  3. parce que c'est incorrect
Les objections à ces arguments sont d'intéressantes.

En ce qui concerne le premier point, le développeur fait remarquer qu'il est "peu probable" que les valeurs dépasseront 10 ou 12. De son côté, le chef d'équipe explique que, dans une approche "pragmatique", lorsque ça ne fonctionnera plus, la correction sera apportée.

Je ne peux être d'accord avec ces deux objections. "Peu probable" me semble "peu rassurant". Quant à l'approche qui consiste à réparer lorsque ça posera problème, elle risque d'entraîner une perte de temps pour retrouver où ça ne marche pas.

En ce qui concerne le deuxième point, c'est simple. Puisque nous (la cellule d'architecture) recommandons l'usage de valueOf plutôt que de new Integer(), la problème ne se présentera pas.

Quant à la troisième réponse, l'objection est simple: en quoi est-ce une erreur, puisque ça marche?

Le plus terrible dans cette histoire, c'est que la correction aurait pris, commit sur Subversion compris, une trentaine de secondes. Notre discussion a duré un quart d'heure.

Alors, c'est quoi finalement la qualité? C'est un sujet sur lequel j'aurai l'occasion de revenir.