How do I use polymorphism instead of instanceof?

Question

I'm trying to make an abstract board game. In the game, a player can choose to make multiple actions within one turn such as placing, moving, or rotating a piece. I'm not sure if whether or not my implementation is bad. I was told by my professor that using instanceof and if statements like this is a sign of code smell, but I can't really think of any other way of implementing this.

My current motivation for implementing the game like this is to be able to dynamically order the different actions into a list so that the board can execute the actions. However, each action is different and has to be executed differently. So since I have to be able to list each action in any order and execute each action, this was the best way that I could think of.

    public class Board {

        private Square[][] squares;

        public void execute(List<Action> actions) {
            for (Action action : actions) {
                if (action instanceof MovingAction) {
                    // 
                } else if (action instanceof RotatingAction) {
                    //
                }
            }
        }
    }


    public interface Action {
    }

    public class MovingAction implements Action {

        private Position positionOfPiece;
        private Position targetPosition;

        public Position getPositionOfPiece() {
             return positionOfPiece;
        }

        public Position getTargetPosition() {
             return targetPosition;
        }

        public MovingAction(Position positionOfPiece, Position targetPosition) {
             this.positionOfPiece = positionOfPiece;
             this.targetPosition = targetPosition;
        }

    }

    public class RotationAction implements Action {

         private Position positionOfPiece;
         private Orientation orientation;

         public RotationAction(Position positionOfPiece, Orientation orientation) {
              this.positionOfPiece = positionOfPiece;
              this.orientation = orientation;
         }

         public Position getPositionOfPiece() {
              return positionOfPiece;
         }

         public Orientation getOrientation() {
              return orientation;
         }

    }

So is there a better way to implement this or is this the best way to do this?

Answer 1

The instanceof checks are a code smell. Essentially, instead of keeping all the logic within the interface and it's implementations you are requiring the calling code to know what type of action is there instead of simply using it. I think it will become a bit more clear as we go on.

First, you want to define the methods that you intend for your actions to implement in the interface. It would look like this:

public interface Action {
    void execute(Square[][] squares);
}

By defining the method you expect your Action to perform, you can simply call it. Now your action processing method in the board is simplified to this:

public void execute(List<Action> actions) {
    for (Action action : actions) {
        action.execute(squares);
    }
}

At this point your Action classes are responsible for implementing what you used to have in the instanceof if then checks and keeping it with the action that is doing the work. I'll just put one of your action types below for illustration:

public class MovingAction implements Action {

    private Position positionOfPiece;
    private Position targetPosition;

    public MovingAction(Position positionOfPiece, Position targetPosition) {
         this.positionOfPiece = positionOfPiece;
         this.targetPosition = targetPosition;
    }

    public void execute(Square[][] squares) {
         Square piece = findSquare(positionOfPiece, squares);
         Square target = findSquare(targetPosition, squares);

         target.setValue(piece.getValue());
         piece.setValue(null);
    }

    private Square findSquare(Position position, Square[][] squares) {
         // ...
    }
}

This approach now makes it easier to add new actions without needing to change the way the Board works at all. Each action is fully self-contained.

At this point, you can start thinking about common logic that you would need to support your actions like the findSquare() method I defined above. That method probably can be a pure function (everything needed to process is passed in, and the result is the same with the same inputs). But sometimes, you might have common code that can be pushed down into a base class. The base class could hold the findSquare() function and now all Actions can use it without redefining it in every class.

Your instructor wasn't criticizing the functionality, but suggesting that you use a design that doesn't require the Board to know how to implement actions. Essentially your actions would become truly active rather than just being passive data containers. It also allows you to hide the internal state of your Action since it doesn't really need to be exposed.