Vous aimez ce que vous lisez sur ce blog ?
Envie d'aller plus loin avec véritable formation d'expertise en Java ?
Venez suivre ma formation Masterclasse Expertise Java !

"Même un développeur experimenté a besoin de continuer à apprendre. Et dans cette formation... j'ai appris beaucoup !" - A.G., Java Champion

Sessions intra-entreprises sur demande : contact[at]mokatech.net.
Inscrivez-vous vite !
15
mar.
2009

Collection accessors considered harmful

Java / JEE › Articles | Tags : java Par Olivier Croisier

L'art du design objet repose sur la détermination des rôles et responsabilités de chaque classe. Quelle classe est responsable de la gestion de telle ou telle donnée ? A qui et sous quelles conditions ces données sont-elles exposées ? Il est bon de se poser ces questions souvent et de questionner son modèle objet tout au long de sa conception.

Actuellement, la mode est aux POJOS. Depuis quelques années, depuis Hibernate et Spring, tout le monde a appris à programmer comme ça. On crée une classe, on déclare ses champs privés, et on génère des accesseurs : un coup de bouton droit dans Eclipse, et le tour est joué - easy as pie.
Le problème, c'est que ce réflexe conditionné est dangereux dans le cas des collections, car il affaiblit l'encapsulation de la classe.

Le code suivant illustre le problème.
En fournissant des accesseurs directs sur la collection d'Items, la classe Caddie expose son fonctionnement interne, et perd le contrôle de l'ajout ou de la suppression d'éléments dans le Set. Non seulement l'encapsulation est rompue, mais il devient plus difficile de changer l'implémentation de la classe, par exemple pour modifier le type de collection utilisée.

  1. public class Caddie
  2. {
  3. 	private Set<Item> items = new LinkedHashSet<Item>();
  4.  
  5. 	public Set<Item> getItems()
  6. 	{	return this.items;
  7. 	}
  8.  
  9. 	public void setItems(Set<Item> items)
  10. 	{	this.items = items;
  11. 	}		
  12. }

La solution suivante permet de reprendre le contrôle sur la collection d'Items. Elle est basée sur deux principes :

  • La collection exposée est une copie en lecture seule. Son type est Collection afin de découpler son implémentation interne de sa représentation externe.
  • Des méthodes sont fournies (addItem() et removeItem()), qui permettent de gérer les Items. Mais cette fois, il est nécessaire de passer par la classe Caddie, qui reprend donc le contrôle de ses données. De plus, son implémentation est désormais masquée, améliorant son encapsulation et sa maintenabilité.
  1. public class Caddie
  2. {
  3. 	private Set<Item> items = new LinkedHashSet<Item>();
  4.  
  5. 	public Collection<Item> getItems()
  6. 	{	return Collections.unmodifiableSet(items);
  7. 	}
  8.  
  9. 	public boolean addItem(Item item)
  10. 	{	return items.add(item);
  11. 	}
  12.  
  13. 	public boolean removeItem(Item item)
  14. 	{	return items.remove(item);
  15. 	}		
  16. }

La prochaine fois que vous générez des accesseurs pour vos POJOs, pensez à porter un regard critique sur leur encapsulation : ne laissez pas Eclipse réfléchir pour vous !

Commentaires

1. Le lundi 16 mars 2009, 00:32 par HollyDays

Quelques remarques :

  • La mode récente des POJO a effectivement fait oublier qu'un POJO n'encapsule pas grand chose. Définir ses DataBean uniquement avec des POJO révèle donc un grave problème de conception -- conception dans laquelle l'encapsulation devrait être un élément fondamental.

    Car il y a tout aussi grave que le fait que l'évolution du code soit rendue plus difficile. La mise au point du programme l'est aussi : car ici, en fournissant un accès public complet à sa collection d'items, la classe Caddie n'a plus aucun contrôle sur le contenu de cette collection. L'utilisateur de cette classe peut à loisir ajouter, supprimer, remplacer le contenu de la collection sans que l'instance de Caddie n'en sache rien. Si le contenu de la collection devient invalide (à cause d'un bug), n'importe quelle instruction du programme, aussi complexe soit-il, est potentiellement responsable (alors qu'avec une bonne encapsulation, seule la classe Caddie est responsable). Bref, un POJO classique, ça n'est pas très différent d'un tuple dont tous les attributs seraient publics...
  • Ce problème ne se pose pas que pour les collections, même les collections en constituent un très bon exemple (et en pratique, elles en sont effectivement un exemple récurrent). En fait, le problème se pose pour tout type d'objet qui appartient à une arborescence de classes.
  • Un outil comme FindBugs détecte ce genre de problèmes, ce qui est très utile. FindBugs peut également se télécharger sous la forme d'un plugin Eclipse. Je vous en recommande donc l'usage en toutes circonstances !

Dernier point : on va sans doute me trouver très pointilleux, mais dans la solution proposée pour getItems() ci-dessus, je vois encore 2 soucis :

  • Un souci de performance : chaque appel à getItems() crée un objet supplémentaire (le unmodifiableSet). Ça vaut sans doute le coup de stocker ce set non modifiable en attribut, et de le créer une fois pour toutes, car il ne fait qu'encapsuler un set qui, lui, est modifiable et sur lequel on dispose toujours, en interne, d'une référence...
  • Un souci d'encapsulation : faire retourner une Collection à getItems() expose encore des informations sur la manière dont est stocké en interne l'ensemble des items (c'est une collection ; donc, ce n'est pas un tableau, par exemple). Se contenter d'exposer le fait que les items soient Iterables ne suffirait-il pas ? (ce qui, d'ailleurs, au passage, éliminerait le besoin de retourner une version non modifiable de la collection.) La réponse à ce genre de questions n'est jamais évidente, et mérite toujours d'être posée !
2. Le lundi 16 mars 2009, 01:08 par Olivier Croisier

Remarques intéressantes, mais concernant le dernier point, je me permets de lever deux objections :

  • Renvoyer un itérateur reviendrait à donner aux classes externes un accès direct à la collection des Items ; en appelant remove() par exemple, elles pourraient modifier la collection. L'encapsulation serait donc compromise.
  • Ensuite, il ne faut pas oublier que cela empêcherait la classe Caddie de modifier ses propres items à sa guise : impossible de modifier le Set alors qu'il est en train d'être itéré (ce qui a d'autant plus de chances de se produire que le nombre d'accès concurrents est élevé), sous peine de récolter une ConcurrentModificationException.

Je reste donc sur ma position quant au renvoi d'une collection en lecture seule, pouvant être itérée sans impacter la classe Caddie - mais qui pourrait, en effet, être initialisée précocément pour des raisons d'optimisation.

3. Le lundi 16 mars 2009, 13:44 par jraduget

Je partage votre point de vue, concernant le manque de "responsabilités" présent dans les implémentation de POJO actuelle.

Par contre avec la démocratisation des annotations jpa, hibernate, ou autre orm... comment procéder sur un bean POJO possédant une collection de ce type ?

Il faut peut être annoter (@Column) la variable d'instance privé de la collection, et non son getter, pour ensuite gérer les accesseurs à la collection avec votre méthode, add & remove ?

Il serait vraiment intéressant de dérouler votre point de vue avec un exemple de bean à rendre persistant :)

Merci

4. Le lundi 16 mars 2009, 18:48 par Olivier Croisier

En effet, il est possible de configurer Hibernate pour qu'il accède directement aux champs par réflexion, sans passer par des setters. Mais du coup, il serait impossible d'initialiser précocément le wrapper en lecture seule.

5. Le lundi 16 mars 2009, 22:38 par HollyDays

Apparemment, tu as mal lu mon post, Olivier. Je ne proposais pas de retourner un Iterator, mais un Iterable. Ce qui est une interface plus restrictive (ou plus générale, si tu préfères) que celle que tu proposes, Collection.

Mais je me dois de battre ma coulpe : comme on peut écrire une boucle foreach aussi bien sur un objet de type Iterable que sur un tableau, j'en avais conclu que tout type tableau de T était convertible en Iterable<T>. Ce qui, vérification faite, n'est pas le cas. Donc on ne peut pas stocker les items du Caddie dans un tableau tout en déclarant que getItems() retourne un objet de type Iterable (sauf à appeler Arrays.asList() sur le tableau et à retourner le résultat, évidemment ; ce qui revient grosso modo à la solution que tu proposes, les ajouts et les suppressions d'items en moins).

A noter que le point que tu soulèves sur Iterable.remove() est assez facile à contourner : nombreuses sont les implémentations d'Iterator dont l'opération remove se contente de lancer systématiquement UnsupportedOperationException.

Enfin, le point le plus important : la solution que tu proposes ne résout en rien le problème de concurrence d'accès que tu soulèves (itérer sur la collection empêche la classe Caddie de modifier la collection en même temps). La collection non modifiable n'est pas une copie de la collection d'origine : c'est la même collection. Elle est seulement encapsulée par un objet qui implémente l'interface Collection (ou Set ou List, ...) et dont les méthodes de modification lèvent systématiquement une exception. Autrement dit, itérer sur la collection non modifiable ne fera rien d'autre qu'itérer sur la collection sous-jacente, qui, elle, est modifiable, et donc on ne pourra pas plus lui ajouter ou lui supprimer un élément dans le même temps.

6. Le lundi 16 mars 2009, 22:52 par HollyDays

A jraduget

La réponse est assez simple en fait : il faut que les POJO annotés Hibernate restent des objets qui représentent les données persistantes, et il ne faut pas qu'ils servent de modèle métier principal en mémoire. En d'autres termes, les POJO annotés Hibernate doivent se contenter d'implémenter la logique d'accès aux données persistantes, et ce sont d'autres classes qui doivent implémenter la logique métier (que l'on peut implémenter sous la forme d'EJB Entité), sur laquelle se repose la logique applicative (que l'on peut implémenter sous la forme d'EJB Session). On peut alors être plus lâche sur l'encapsulation des POJO annotés Hibernate, dès lors que leur usage reste limité et restreint au niveau du source.

Rien n'empêche d'ailleurs que les classes qui implémentent la logique métier (autrement dit, le modèle métier en mémoire) ne fassent qu'encapsuler les objets annotés Hibernate et fassent lourdement appel au mécanisme de délégation (ça évite notamment les recopies d'information, sous la forme de palanquées de setXxx(), et les risques d'incohérence en mémoire entre les objets du modèle métier et ceux du modèle d'accès aux données). L'important étant, encore une fois, de bien encapsuler pour maîtriser le plus possible l'interface publique des classes que l'on définit et que l'on utilise.

7. Le lundi 16 mars 2009, 23:43 par Olivier Croisier

Errr, exact. Pour ma défense, je plaide la fatigue à 2h du mat'...

Le problème des accès concurrents est aisément résolu soit en remplaçant le HashSet par un CopyOnWriteArraySet (qui crée une nouvelle copie de son tableau interne à chaque modification), soit en créant un nouveau Set à chaque appel de getItems() et en le wrappant dans un unmodifiableSet. En fonction du ratio lectures/écritures, on choisira l'une ou l'autre, mais dans l'ensemble, les perfs ne devraient pas être un problème, seules les références étant recopiées. En tout cas, ces deux solutions sont proposées dans ma bible du moment, Concurrency In Action.

M'enfin, ça ne change rien au message initial qui était qu'il fallait se méfier des accesseurs sur les collections...

Concernant Hibernate, je suis plutôt d'accord. On peut tout à fait considérer que les classes mappées sont de simples DTO au même titre que ceux utilisés pour les communications avec les systèmes distants ou avec la couche de présentation. A partir de là, si ces classes sont correctement isolées des "vrais" objets métiers, il est possible d'assouplir leurs contraintes.

Ajouter un commentaire

Le code HTML est affiché comme du texte et les adresses web sont automatiquement transformées.

Propulsé par Dotclear | Icônes FamFamFam