Avoiding instanceof operator in this case

Question

I am writing a simple game with cells that can be in two states Free and Taken .

interface Cell { int posX(); int posY(); } abstract class BaseCell implements Cell { private int x; private int y; public int posX() { return x; } public int posY() { return y; } ... } class FreeCell extends BaseCell { } class TakenCell extends BaseCell { private Player owningPlayer public Player owner() { return owningPlayer; } } 

In each case, I need to check all the cells to calculate the next state of the cell using the method below

 // method in class Cell public Cell nextState(...) {...} 

and collect (in Set ) all the cells that are not yet accepted. The above method returns Cell because the cell can change from Free to Taken or vice versa. I am doing something like below to assemble them:

 for (Cell cell : cells) { Cell next = cell.futureState(...); if(next instanceof FreeCell) { freeCells.add(currentCell); } ... } 

It's not beautiful. How to do this to avoid such examples of hacks? I'm not talking about another hack, but I would like to find the right solution for OOP.

Answer 1

Ok, here is another approach you can take.

  public class Cell { private int x; private int y; private OccupationInfo occupationInfo; public int posX() { return x; } public int posY() { return y; } public OccupationInfo getOccupationInfo() { return occupationInfo; } public boolean isFree() { return occupationInfo == null; } } 

And then...

  public class OccupationInfo { private Player owningPlayer; // any other data you would've put in `TakenCell` } 

It may or may not be good for your specific purposes, but it is a clean and simple design.

Answer 2

It sounds like you are flirting with a "State" pattern , but you're not quite there. Using the state template, you will have your own Cell object and the class hierarchy "State State".

The Cell object will use composition, not inheritance. In other words, the cell will have the current state property. When you have a cell where the currentState property is a FreeState object, then this is a free cell. When you have a cell where the currentState property is a TakenState object, then this is a free state.

How to do this to avoid such examples of hacks?

Whenever you have a situation where you need to make an instance, you add a method to your Cell class and just call it. The cell delegates the current state. The code in the cell that delegates to the current state does not actually know what the state is. He simply believes that the state will do the right thing. In FreeState and TakenState, you provide implementations of each method that do the right thing based on their state.

Answer 3

I think the design problem here is that you have two different classes for what can be essentially two different states of the same cell.

What do you do now when a previously free cell becomes busy? Create a new object with the same coordinates and discard the old one? But this is still the same cell conceptually! (Or can there be a free cell and a taken cell with the same x and y at the same time?)

From an OOP point of view, you should have one cell class with the attribute “taken” or, as another user suggests, “owner information”. If you think that this should not be part of the cell class for any reason, how about storing owner information separately in Map<Cell,Owner> ?

Answer 4

I think it's nice to use a Factory Template or an Abstract Factory Template .

The factory pattern returns an instance of several (product hierarchy) subclasses (e.g., FreeCell, TakenCell, etc.), but the calling code is not aware of the actual implementation class.
The calling code calls a method on an interface, such as FreeCell , and the correct doSomething () method is called using polymorphism.

Instead of using instanceof (e.g. switching), you can just call the same method, but each class will implement it according to a local override. This is a very powerful and common feature in many contexts.

Instead, write:

 for (Cell cell : cells) { Cell next = cell.futureState(...); if(next instanceof FreeCell) { freeCells.add(currentCell); } ... } 

You can enter:

 for (Cell cell : cells) { Cell next = cell.futureState(...); cell.doSomething(); // and no matter what class is FreeCell or TakenCell ... 

}

Factory pattern returns one of several subclasses. You should use a factory pattern. If you have a superclass and several subclasses, and based on some of the data provided, you should return an object from one of the subclasses.

References:

Abstract Factory Template

Factory pattern

Answer 5

You can add a method to the Cell interface that will determine if the cell is free:

 interface Cell { int posX(); int posY(); boolean isFree(); } class FreeCell extends BaseCell { public boolean isFree() { return true; } } class TakenCell extends BaseCell { private Player owningPlayer public boolean isFree() { return false; } public Player owner() { return owningPlayer; } } 

But I don't think this is much better than using an instance.

Answer 6

Do you have two sets and move cells from one to another when they are taken? For example, at the beginning you will have a freeSet full of cells, and takenSet an empty one. When cells are taken, they move from the freeSet to the accepted set. If you have the interface above TakenCell and FreeCell, you can enter each set with the same interface.

On the other hand...

It would be useful to see your definition of FreeCell and TakenCell , but I would think that you could model them as the same object with a field with a zero value that will be populated to indicate that it is accepted. Then you can use two types of dialing for the same class.

Answer 7

Your code is not ugly, it is well read, it clearly expresses the logic of the application.

The only instanceof test is usually nothing to worry about; its use is general. Token interfaces are tested on instanceof . Your example is a kind of marker interface.

And instanceof insanely fast, obviously, the JVM considers it appropriate to optimize it very well.

However, the instanceof test chain may be a sign of a problem. Add a protector to make sure the listing is complete.

 if(o instanceof A) ... else if(o instanceof B) ... else if ... ... else // huh? o is null or of unknown type throw new AssertionError("unexpected type: "+o); 

Answer 8

you can use the visitor pattern , these FreeCell and TakenCell should implement

Visible interface

 interface Visitable { void accept(Visitor visitor); } interface Visitor { void visit(FreeCell freeCell); void visit(TakenCell takenCell); } 

in the implementation of visiting the visitor (FreeCell freecCell method) will be:

 public void visit(FreeCell freeCell) { freeCells.add(freeCell); } 

there will be nothing in implementing a visitor visit (TakenCell tookCell method)

and both classes: FreeCell and TakenCell, in the accept method (visitor visitor) should have:

 public void accept(Visitor visitor) { visitor.visit(this); } 

and in the for loop you should have:

 for (Cell cell : cells) { Cell next = cell.futureState(...); next.accept( someConcreteVisitor ) ... } 

someConcreteVisitor is an instance of a visitor implementer.

the class where it is for the loop can also be Visitable.