Getting rid of `instanceof`

Question

In writing the sprites I write, each field in the 2D grid contains a stack of sprites. Mainly considered the top.

I have a lot of code in the game rules module:

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

---

Upadate: //Do something counts if there is a Box on top of each Target . I don’t see how this can be done by simply adding doSomething() to Sprite, unless doSomething() returns 1, if the sprite is a field, and 0 otherwise. (and it will be exactly the same as instanceof).

---

I know that instanceof is considered harmful because it kills the idea of ​​object-oriented programming.

However, I am not sure how to fix the code in my case. Here are some thoughts that I had:

• I do not think it is easier to add the isABox() method to the Sprite interface.
• Would it help if Box was an interface, so could other classes get the same privilege?
• Should I try to do something unusual, like pattern matching / dual submit, with visitor patterns?
• Is it good that the rule module works with types, simply because it needs to know its semantics anyway?
• Does the whole idea of ​​a rule module strategy template impede?
• It makes no sense to create rules in Sprites, since then all of them must be changed when adding a new type.

Hope you tried something similar and can point me in the right direction.

Answer 1

Here is my attempt. Consider the definition of an enumeration with different types of sprites:

 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; } } 

And finally:

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

Thus, you can solve the problem of creating the isATypeX () method in Sprite for each type X. If you need a new type, you add a new value to the enumeration, and only the rule that needs to check this type should know it.

Answer 2

Use polymorphism :

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

So you just need to call sprite.someMethod () in your example.

Answer 3

Instanceof : (almost) always harmful

I looked at all the answers to your post and tried to understand what you are doing. And I came to the conclusion that Instanceof exactly what you want, and your original code sample was fine.

You explained that:

• You do not violate the Liskov substitution principle, since none of the Box codes cancels the Sprite code.

• You are not to deploy the Instanceof answer code. This is why people say instanceof is bad; because people do this:

   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).triangleArea(); } 

  This is why people shun instance. But that is not what you do.

• There is a one-to-one relationship between Box and winning the game (no other Sprites can win the game). This way, you don’t need an additional abstraction of the “winner” sprite (because Boxes == Winners).

• You simply check the board to make sure that each top element is a field. This is exactly what Instanceof needs to do.

Everyone else will answer (including my own), adds an extra mechanism to check if Sprite is a field. However, they do not add any stability. In fact, you use functions that are already supplied by the language and override them in your own code.

Tomas Narros argues that you must distinguish between "semantic types" and "Java types" in your code. I do not agree. You have already established that you have a java type, Box , which is a subclass of Sprite . Thus, you already have all the necessary information.

In my opinion, the presence of a second independent mechanism, which also reports “I am a box,” violates DRY (Do not Repeat Yourself). This means the absence of two independent sources for the same information. Now you need to save the enumeration and structure of the class.

The so-called "benefit" is the ability to pirouette around a keyword that completely fills the target and is harmful when used in more malicious ways.

---

Golden Rule Use your head . Do not obey the rules as a difficult fact. Ask them, find out why they are there, and bend them when necessary.

Answer 4

Basic overload is the way to go. This is a Sprite class hierarchy that needs to know what to do and how to do it, for example:

 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: Your change of question does not fundamentally change the problem. You have some kind of logic that applies only to Box . Again, encapsulate this specific logic in a Box instance (see above). You can go further and create a general superclass for your sprites that defines the default value for isCountable() (note that the method is similar to isBox, but actually better from a design point of view, since there is no point in having a isBox method for a circle - should Box also contain the isCircle method?).

Answer 5

In principle, instead of

 if (sprite instanceof Box) // Do something 

Using

 sprite.doSomething() 

where doSomething() is defined in Sprite and redefined in Box .

If you want these rules to be separate from the hierarchy of Sprite classes, you can move them to a separate Rules class (or interface), where Sprite has a getRules() method, and subclasses return different implementations. This will further increase flexibility by allowing objects of the same Sprite subclass to have different behavior.

Answer 6

Below is an example of a common counter without the isAnX () method for each type that you can count. Say you want to count the number of types of X on a board.

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

---

I suspect you really want

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

Answer 7

What are you really testing here

Can a player win a game with this Sprite at the top of the board?

Therefore, I suggest these names:

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

It makes no sense to have an isBox function. No way. You can also use instanceof .

But if Box , Bottle and Target are all winning fragments, you can return them to everyone

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

Then you can add another type of “winning” sprite without changing the isGameWon function.

Answer 8

How about using a visitor.

Each point inherits the acceptBoxDetection method:

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

Answer 9

General statements about object-oriented design / refactoring are difficult to give IMHO, since the "best" action is very dependent on the context.

You should try moving “Do something” to a virtual Sprite method that does nothing. This method can be called from your loop.

The box can then override it and do "something."

Answer 10

I think people suggested you the right ones. Your doSomething() might look like this:

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

So in your source code you can do this:

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

Or else you can also achieve the same goal with what you have proposed, but it may cost an additional calculation per cycle.

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

Answer 11

When you say “stack” and “top count”, could you just take the top one and do something with it?