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?

  • 1
    What do you want to be "better?" – Robert Harvey Oct 31 at 23:06
  • @RobertHarvey: At a guess, I'd say OP wants to move the code which currently lives in various if/else if branches in Board.execute() into a method under the Action interface. If it needs to mutate the Board directly, then Board may need to grow new methods to accommodate that. – Kevin Oct 31 at 23:35
  • 1
    @RobertHarvey Honestly, I'm not sure. I thought the way I did this fine. But my professor had a problem with the Board class and how it has to use instanceof and and if statements to actually carry out the actions. I'm assuming he wants me to use polymorphism some how. So I guess my question is how do I force this to use polymorphism? – Michael Newgate Oct 31 at 23:37
  • Well, if your descendant classes implemented the base class's Action method instead of their own custom action methods, you could new up an instance of a descendant class as an Action (i.e. Action myAction = new RotationAction()), call the Action method on the resulting instance, and it would perform the custom action of the class you newed up (in my example, a Rotation action). – Robert Harvey Nov 1 at 0:10
  • @RobertHarvey: I get it now. So I can just make a method that takes a Board as an argument in Action. Then all I have to do in the class Board is pass the board into the action's method in the execute method so that action can operate on the board that I passed in. And as Kevin mentioned, I need to add extra methods to the class Board so that the different actions can do what they need to do. Thank you for the help. But out of curiosity, is the original way that I wrote this bad code? My professor made it seem like I should never do that. – Michael Newgate Nov 1 at 0:47
up vote 6 down vote accepted

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.

  • That makes a lot of sense. I think I get the motivation for why I should code it like this. Thank you for breaking it down for me. – Michael Newgate Nov 1 at 1:04
  • No problem. Just paying forward the help I got when I started out. – Berin Loritsch Nov 1 at 1:07
  • The solution is right, but the follow-up suggestions conflict with object-orientation. Both "pure functions" and "base classes" (for the purpose of sharing "common" code) is an anti-pattern in oo. – Robert Bräutigam Nov 2 at 11:07
  • @RobertBräutigam, pure functions exist in every environment. There's only one real definition of a square root for example. For languages that don't support pure functions, they are anchored as a static method on a static class like Math.sqrt(val) in the standard libraries for the language themselves. Saying they are an antipattern is denying reality. – Berin Loritsch Nov 2 at 12:52
  • @RobertBräutigam, I'll give you the pushing common code to a base class just for that purpose. In all honesty, use of inheritance does need to be considered thoughtfully, and most of the time is not truly needed--particularly if the language supports interfaces. – Berin Loritsch Nov 2 at 12:54

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.