Se débarrasser de `instanceof`

Dans un jeu à base de sprites que j’écris, chaque champ d’une grid 2D contient une stack de sprites. Ce sont surtout les meilleurs qui comptent.

Dans le module de règles du jeu, j’ai beaucoup de code comme celui-ci:

public boolean isGameWon(Board board) { for (Point point : board.getTargetPoints()) if(!(board.getTopSpriteAt(point) instanceof Box)) return false; return true; } 

Upadate: //Do something compte s’il y a une Box sur chaque Target . Je ne vois pas comment cela pourrait être fait en ajoutant simplement doSomething() à Sprite, à moins que doSomething() retourne 1 si le sprite est une boîte et 0 sinon. (et ce serait la même chose que instanceof).


Je sais que instanceof est considéré comme dangereux car il détruit l’idée de la programmation orientée object.

Je ne suis toutefois pas sûr de savoir comment corriger le code dans mon cas. Voici quelques reflections que j’ai eues:

  • Je ne pense pas qu’il soit préférable d’append simplement une méthode isABox() à l’interface Sprite .
  • Est-ce que cela aiderait si Box était une interface, afin que d’autres classes puissent obtenir le même privilège?
  • Devrais-je essayer de faire quelque chose d’extraordinaire tel que correspondance de modèle / double envoi, avec des modèles de type visiteur?
  • Est-ce normal que le module de règles travaille intimement avec les types, simplement parce qu’il est censé connaître leur sémantique de toute façon?
  • L’idée d’un modèle de stratégie de module de règles est-elle erronée?
  • Il n’a aucun sens de construire les règles dans les Sprites, car il faudrait alors toutes les changer lorsqu’un nouveau type est ajouté.

J’espère que vous avez essayé quelque chose de similaire et êtes capable de me diriger dans la bonne direction.

Voici mon essai. Pensez à définir une énumération avec les différents types de Sprite:

 class Sprite { public enum SpriteType { BOX, CAT, BOTTLE, HUMAN, TARGET, /* ... */, SIMPLE; } public SpriteType getSpriteType(){ return SIMPLE; } } class Box extends Sprite { @Override public SpriteType getSpriteType(){ return Sprite.SpriteType.BOX; } } 

Et enfin:

 public boolean isGameWon(Board board) { for (Point point : board.getTargetPoints()) if(board.getTopSpriteAt(point).getSpriteType()!=SpriteType.BOX) return false; return true; } 

De cette façon, vous pouvez résoudre le problème de devoir créer une méthode isATypeX () dans Sprite pour chaque type X. Si vous avez besoin d’un nouveau type, vous ajoutez une nouvelle valeur à l’énum, ​​et uniquement la règle devant vérifier ce type. aura besoin de le savoir.

Utiliser le polymorphism :

 class Sprite { .. someMethod(){ //do sprite } .. } class Box extends Sprite { .. @Overrides someMethod(){ //do box } .. } 

Donc, il vous suffit d’appeler sprite.someMethod () dans votre exemple.

Instanceof of: (Presque) toujours nocif

J’ai jeté un coup d’œil à toutes les réponses à votre message et j’ai essayé de comprendre ce que vous faisiez. Et j’en suis venu à la conclusion instanceof est exactement ce que vous voulez et que votre exemple de code initial était correct.

Vous avez précisé que:

  • Vous ne violez pas le principe de substitution de Liskov car aucun des codes Box n’invalide le code Sprite.

  • Vous ne communiquez pas le code avec la réponse à instanceof . C’est pourquoi les gens disent qu’instanceof est mauvais; parce que les gens font ceci:

     if(shape instanceof Circle) { area = Circle(shape).circleArea(); } else if(shape instanceof Square) { area = Square(shape).squareArea(); } else if(shape instanceof Triangle) { area = Triangle(shape).sortingangleArea(); } 

    C’est la raison pour laquelle les gens évitent l’instance de. Mais ce n’est pas ce que vous faites.

  • Il y a une relation un-à-un entre Box et gagner le jeu (aucun autre Sprite ne peut gagner le jeu). Vous n’avez donc pas besoin d’une abstraction de sprite “gagnante” supplémentaire (car Boxes == Winners).

  • Vous vérifiez simplement le tableau pour vous assurer que chaque élément supérieur est une boîte. C’est exactement ce que instanceof est conçu pour faire.

La réponse de tous les autres (y compris la mienne) ajoute un mécanisme supplémentaire permettant de vérifier si un Sprite est une Box. Cependant, ils n’ajoutent aucune robustesse. En fait, vous prenez des fonctionnalités déjà fournies par la langue et vous les réimplémentez dans votre propre code.

Tomas Narros affirme que vous devez distinguer dans votre code les “types sémantiques” et les “types java”. Je ne suis pas d’accord. Vous avez déjà établi que vous avez un type Java, Box , qui sous-classe Sprite . Donc, vous avez déjà toutes les informations dont vous avez besoin.

À mon avis, le fait d’avoir un deuxième mécanisme indépendant qui signale également “Je suis une boîte” viole DRY (Don’t Repeat Yourself). Cela signifie ne pas avoir deux sources indépendantes pour la même information. Vous devez maintenant maintenir une enum et une structure de classe.

Le “bénéfice” est la possibilité de pirater un mot clé qui remplit pleinement son objective et qui est préjudiciable lorsqu’il est utilisé de manière plus préjudiciable.


La règle d’or est d’ utiliser votre tête . Ne pas obéir à des règles comme fait dur Interrogez-les, découvrez pourquoi ils sont là et pliez-les le cas échéant.

La surcharge de base est la voie à suivre ici. C’est la hiérarchie des classes Sprite qui doit savoir quoi faire et comment le faire, comme dans:

 interface Sprite { boolean isCountable(); } class MyOtherSprite implements Sprite { boolean isCountable() { return false; } } class Box implements Sprite { boolean isCountable() { return true; } } int count = 0; for (Point point : board.getTargetPoints()) { Sprite sprite = board.getTopSpriteAt(point); count += sprite.isCountable() ? 1 : 0; } 

EDIT: votre modification de la question ne modifie pas fondamentalement le problème. Ce que vous avez, c’est une logique qui ne s’applique qu’à Box . Encore une fois, encapsulez cette logique particulière dans l’instance Box (voir ci-dessus). Vous pourriez aller plus loin et créer une superclasse générique pour vos sprites définissant une valeur par défaut pour isCountable() (notez que la méthode est similaire à celle d’isBox mais est en réalité meilleure d’un sharepoint vue de la conception, car cela n’a aucun sens pour un cercle de avoir une méthode isBox – Box doit-il également contenir une méthode isCircle?).

Fondamentalement, au lieu de

 if (sprite instanceof Box) // Do something 

Utilisation

 sprite.doSomething() 

doSomething() est défini dans Sprite et annulé dans Box .

Si vous souhaitez que ces règles soient séparées de la hiérarchie des classes Sprite, vous pouvez les déplacer vers une classe (ou interface) de Rules séparée, où Sprite dispose d’une méthode getRules() et où les sous-classes renvoient des implémentations différentes. Cela augmenterait encore la flexibilité, car cela permettrait aux objects de la même sous-classe Sprite d’avoir un comportement différent.

Voici un exemple de compteur générique sans méthode isAnX () pour chaque type à compter. Dites que vous voulez compter le nombre de type X sur le tableau.

 public int count(Class type) { int count = 0; for (Point point : board.getTargetPoints()) if(type.isAssignable(board.getTopSpriteAt(point))) count++; return count; } 

Je soupçonne que tu veux vraiment

 public boolean isAllBoxes() { for (Point point : board.getTargetPoints()) if(!board.getTopSpriteAt(point).isABox()) return false; return true; } 

Ce que vous testez vraiment ici, c’est

Le joueur peut-il gagner la partie avec ce Sprite en haut du tableau?

Par conséquent, je suggère ces noms:

 public boolean isGameWon(Board board) { for (Point point : board.getTargetPoints()) if(!board.getTopSpriteAt(point).isWinningSprite()) return false; return true; } 

Il ne sert à rien d’avoir une fonction isBox . Pas du tout. Vous pourriez aussi bien utiliser instanceof .

Mais si Box , Bottle et Target sont tous des tuiles gagnantes, vous pouvez toutes les faire revenir.

 class Box { public override bool isWinningSprite() { return true; } } 

Vous pouvez ensuite append un autre type d’image-object “gagnant” sans modifier la fonction isGameWon .

Que diriez-vous d’utiliser un visiteur.

Chaque point hérite de la méthode acceptBoxDetection:

 boolean acceptBoxDetection(Visitor boxDetector){ return boxDetector.visit(this); } and then Visitor does: boolean visit(Box box){ return true; } boolean visit(OtherStuff other){ return false; } 

Les déclarations générales sur la conception / refactorisation orientée object sont difficiles à donner à mon humble avis car la “meilleure” action dépend beaucoup du contexte.

Vous devriez essayer de déplacer le “faire quelque chose” dans une méthode virtuelle de Sprite qui ne fait rien. Cette méthode peut être appelée depuis votre boucle.

Box peut alors le remplacer et faire le “quelque chose”.

Je pense que ce que les gens vous ont suggéré est correct. Votre doSomething() peut ressembler à ceci:

 class Sprite { public int doSomething(int cnt){ return cnt; } } class Box extends Sprite { @Override public int doSomething(int cnt){ return cnt + 1; } } 

Donc, dans votre code d’origine, vous pouvez faire ceci:

 int cnt = 0; for (Point point : board.getTargetPoints()) { Sprite sprite = board.getTopSpriteAt(point) cnt = sprite.doSomething(cnt); } 

Sinon, vous pouvez également atteindre le même objective avec ce que vous avez suggéré, mais cela peut coûter un calcul supplémentaire par boucle.

 class Sprite { public boolean isBox() { return false; } } class Box extends Sprite { @Override public boolean isBox(){ return true; } } 

Quand vous dites «stack» et «le premier compte», ne pouvez-vous pas simplement prendre le premier et en faire quelque chose?