getting rid of "instanceof"

Newbie & Debugging Questions
#1

This is the update loop for the game I’m working on:

public void update() {
		Jukebox.play(currSong, true);

		for (int i = 0; i < spawners.size(); i++)
			spawners.get(i).update();
		
		for (int i = 0; i < decorations.size(); i++)
			decorations.get(i).update();

		removeSpawners();
		removeDecorations();
		
		if (levelUp) {
			if (count++ == 60 * 3)
				loadNextGameState();
			return;
		}
		
		if(player.getState() instanceof StatePrimaryPlayerDead) {
			if (count++ == 60 * 2)
				loadNextGameState();
			return;
		}

		player.update();

		if (forceField != null) forceField.update();

		for (int i = 0; i < balls.size(); i++)
			balls.get(i).update();

		int numBricks = 0;
		for (int i = 0; i < bricks.size(); i++) {
			bricks.get(i).update();
			if (bricks.get(i).getState() instanceof BrickDestructibleState) numBricks++;
		}
		if (numBricks == 0) levelUp = true;

		for (int i = 0; i < powerups.size(); i++)
			powerups.get(i).update();

		for (int i = 0; i < projectiles.size(); i++)
			projectiles.get(i).update();

		checkMobCollision();
		removeTargets();
		removeBalls();
		removePowerups();
		removeProjectiles();
		removeForceField();

		if (balls.size() == 0) {
			if (!(player.getState() instanceof StatePrimaryPlayerDead)) {
				player.setState(new StatePrimaryPlayerDead().init(player));
				addSpawner(new ParticleSpawner(player.getx() + player.getWidth() / 2,
						player.gety() + player.getHeight() / 2, 100));
			}
		}
	}

It uses the “instanceof” keyword 3 times, and is used another 3 times in other methods of the same class. In every case it is to detect the state of an entity. I really don’t like coupling the loop to external code like that, but I can’t think of a way to detect entity states without using “instanceof”, and I was looking for suggestions on what to do, as I feel that such extended use of “instanceof” smells of bad design. Any suggestions are greatly appreciated!

#2

Here is another example from one of the states of the Ball entity:

protected void processXCollision(Mob m) {
			
		int xDist = (b.getx() + b.getWidth() / 2) - (m.getx() + m.getWidth() / 2);
		b.setxspeed(Math.abs(xDist / 4));
		
		if (m instanceof Player || m instanceof ForceField) {
			b.setxdir(Util.clamp(xDist, -1, 1));
			if(m instanceof ForceField)
				b.setxspeed(Util.clamp(b.getxspeed(), -3, 3));
		}
		
		else if (m.getState() instanceof BrickSolidState) {
			if (b.getxspeed() == 0) b.setxspeed(Util.random.nextInt(2) > 0 ? -1 : 1);
			if (b.getxdir() == 0) b.setxdir(Util.random.nextInt(2) > 0 ? -1 : 1);
		}
	}

Whenever the ball collides with another mob/entity, it has to check what kind of entity it is (well, actually, it checks the entity’s state), by using “instanceof”. Just wondering if this kind of usage is normal and ok, or if it is bad design and will lead to problems in the future. As far as I can tell, this approach would not scale well, but I could be wrong. Any redesign suggestions would be appreciated if that is what I should do.

#3

Best way to approach this is to make an abstract method in Mob called processXCollision() and have the subclasses define their own way to behave.

#4

Thank you for the reply. Maybe I am misunderstanding you but essentially, that’s what ProcessXCollision() is. ProcessCollision() is an abstract method of State, which is a member of Mob. ProcessXCollision is just a subdivision of that method, i.e. it gets called by ProcessCollision() and is unique to that particular Mob state.

Here’s the declaration:

public interface State {
	
	public State init(Mob m);
	
	public State update();
	
	public void render(Screen s);
	
	public void processCollision(Mob m);
	
	public Mob getMob();
}

So my problem is that the collision is processed differently depending on the Mob that “this” comes in contact with. processCollision() takes Mob as an argument, and the Mob state has to be checked so “this” knows how to properly react, and the only way I know how to do that is with “instanceof”. That ProcessXCollision is unique to the ball’s state BallNormalState:

package com.noah.breakit.entity.mob.ball;

import com.noah.breakit.entity.mob.Mob;
import com.noah.breakit.entity.mob.brick.Brick;
import com.noah.breakit.entity.mob.brick.BrickPortalState;
import com.noah.breakit.entity.mob.brick.BrickSolidState;
import com.noah.breakit.entity.mob.forcefield.ForceField;
import com.noah.breakit.entity.mob.player.Player;
import com.noah.breakit.entity.state.State;
import com.noah.breakit.graphics.Screen;
import com.noah.breakit.sound.SoundFX;
import com.noah.breakit.util.Config;
import com.noah.breakit.util.Util;

public class BallNormalState implements State {
	
	protected Ball b = null;
	
	public State init(Mob m) {
		b = (Ball) m;
		b.setCol(0xff00ff);
		adjustSize(4);
		return this;
	}
	
	public State update() {
		
		b.processWallCollision();
		
		b.updatexa();
		b.updateya();

		if (!b.released) b.setxa(b.getPlayfield().getPlayer().getxa());

		b.movex();
		b.movey();
		
		b.portalSicknessTimer = Util.max(++b.portalSicknessTimer, Ball.PORTAL_SICKNESS_TIME);
		
		return this;
	}

	public void render(Screen s) {
		
		s.fillRect(b.getx(), b.gety(), b.getWidth(), b.getHeight(), b.getCol());
		
		if (b.released) return;
		
		String s0 = "stage-" + (b.getPlayfield().getStage() + 1);
		String s1 = "press <space>";
		String s2 = "to launch!";
		int xofs = Config.WINDOW_WIDTH >> 1;
		int yofs = Config.WINDOW_HEIGHT >> 1;

		s.renderString8x8(xofs - ((s0.length() << 3) >> 1), yofs - 10, 0xffffff, s0);
		s.renderString8x8(xofs - ((s1.length() << 3) >> 1), yofs, 0xffffff, s1);
		s.renderString8x8(xofs - ((s2.length() << 3) >> 1), yofs + 10, 0xffffff, s2);
		
	}

	public void processCollision(Mob m) {

		if (!b.released) return;
		
		if (m instanceof Brick && m.getState() instanceof BrickPortalState) {
			
			BrickPortalState s = (BrickPortalState) m.getState();
			
			if(b.portalSicknessTimer == Ball.PORTAL_SICKNESS_TIME) {
				int xd = Math.abs(b.getx() + b.getWidth() / 2 - m.getx() - m.getWidth() / 2);
				int yd = Math.abs(b.gety() + b.getHeight() / 2 - m.gety() - m.getHeight() / 2);
				if (xd <= 8 && yd <= 4) {
					b.portalSicknessTimer = 0;
					b.setx(s.getMate().getMob().getx() + s.getMate().getMob().getWidth() / 2);
					b.sety(s.getMate().getMob().gety() + s.getMate().getMob().getHeight() / 2);
					b.setydir(b.getydir() * -1);
					SoundFX.PORTAL.play();
				}
			}
			return;
		}
			
		processXCollision(m);
		processYCollision(m);
			
		if (m instanceof Player || m instanceof ForceField) {
			b.setMultiplier(1);
			SoundFX.HI_BOUNCE.play();
		}
	}
	
	protected void processXCollision(Mob m) {
			
		int xDist = (b.getx() + b.getWidth() / 2) - (m.getx() + m.getWidth() / 2);
		b.setxspeed(Math.abs(xDist / 4));
		
		if (m instanceof Player || m instanceof ForceField) {
			b.setxdir(Util.clamp(xDist, -1, 1));
			if(m instanceof ForceField)
				b.setxspeed(Util.clamp(b.getxspeed(), -3, 3));
		}
		
		if (m instanceof Player) {
			b.setxdir(Util.clamp(xDist, -1, 1));
			b.setxspeed(Math.abs(xDist / 4));
		}
		
		else if (m.getState() instanceof BrickSolidState) {
			if (b.getxspeed() == 0) b.setxspeed(Util.random.nextInt(2) > 0 ? -1 : 1);
			if (b.getxdir() == 0) b.setxdir(Util.random.nextInt(2) > 0 ? -1 : 1);
		}
	}
	
	protected void processYCollision(Mob m) {
		if(m instanceof Brick) {
			int yDist = (b.gety() + b.getHeight() / 2) - (m.gety() + (m.getHeight() / 2));
			b.setyspeed(Util.min(Math.abs(yDist / 2), 3));
			if (b.getyspeed() == 0) b.setyspeed(3);
			b.setydir(Util.clamp(yDist, -1, 1));
			if (b.getydir() == 0) b.setydir(-1);
		} else
			b.setydir(-1);
	}

	public Mob getMob() {
		return b;
	}
	
	protected void adjustSize(int size) {
		int wOld = b.getWidth();
		int hOld = b.getHeight();
		b.setWidth(size);
		b.setHeight(size);
		b.setx(b.getx() - (b.getWidth() / 2) + (wOld / 2));
		b.sety(b.gety() - (b.getHeight() / 2) + (hOld / 2));
	}
}

You can see I have many “instanceof” keywords in there. It works fine for this game since it is a very small one, but I’m thinking in terms of larger projects in the future. How can I design this to not use instance of, which seems to be brittle and scale poorly, or am I overthinking this? I’t hard to tell because there really is no manual for something like this.

#5

My current notion for handling “states” is to make use of enums.


    class Ball 
    {
        enum State {NORMAL, SOME_OTHER_STATE};
        State state; 

        public Ball()
        {
            /*
             *  Code that initializes the state variable in the constructor, probably.
             */
        }

        @Override // for an Interface such as one for animatable objects.
        public void update()
        {
            switch (state)
            {
                case NORMAL:
                    doSomething();
                    break;
                case SOME_OTHER_STATE:
                    doSomethingElse();
            }
        }
    }

In other words, I tend to hold variations in state as properties, not as separate subclasses.

If you are going to use subclasses, that seems to me to imply also making collections that are specific to the subclass (e.g., ArrayList) and coding routines specifically for iterations through those collections, rather than testing every Object with instanceof. But if taking a route such as this, I think using Interfaces makes more sense than subclassing.

#6

See “Visitor Pattern”.

Cas :slight_smile:

#7

@philfrei I understand your reasoning, but I just want to make clear that in ProcessCollision(Mob m), the argument is the Mob with which the Mob has collided, not the mob itself.

For example:
Ball extends Mob
BallNormalState implements State

Ball ball
BallNormalState ballState
ball and ballState point to each other, in other words ball “owns” ballState

In ballState.processCollision(Mob m), m is the colliding Mob, not the ball that “owns” ballState. It is either a Brick or the Player. So if I make the state an enum instead of a class, I still have the same problem, where I have to test whether m is the Player or a Brick, and being a total noob, the only way I know to do this effectively is with “instanceof”. I could use some sort of hand-crafted entity type coding, but this would not solve the problem of the code being coupled in the exact same way. Feel free to correct me if I am missing something!

@princec I am researching that now to see if it what I need.

Thank you both!

#8

I haven’t finished refactoring, but so far the visitor pattern has decoupled everything very nicely.

#9

Check out this post from … 2002.

Cas :slight_smile:

#10

OK so I finished refactoring and testing and I have to conclude that it sort of worked. In the sense that I managed to decouple the entities from each other, it absolutely worked. Now the colliding entities don’t care what sort of entity they collided with, but rather accept each other’s visitors and let the pattern do the work. However, no matter what I did, I still couldn’t get rid of “instanceof”. The keyword was simply outsourced to the visitors. I’d call it a half-victory, since the visitors are where the coupling takes place. Just wondering if I’m implementing this correctly. Here is an example visitor:

package com.noah.breakit.visitor;

import com.noah.breakit.entity.mob.Mob;
import com.noah.breakit.entity.mob.ball.Ball;
import com.noah.breakit.entity.mob.brick.Brick;
import com.noah.breakit.entity.mob.brick.BrickDestructibleState;
import com.noah.breakit.entity.mob.forcefield.ForceField;
import com.noah.breakit.entity.mob.player.Player;
import com.noah.breakit.entity.mob.powerup.Powerup;
import com.noah.breakit.entity.mob.projectile.Projectile;

public class ProjectileVisitor implements Visitor {
	
	Projectile projectile = null;
	
	@Override
	public Visitor init(Mob m) {
		projectile = (Projectile) m;
		return this;
	}

	@Override
	public void visit(Ball ball) {		
	}

	@Override
	public void visit(Brick brick) {
		if(!(brick.getState() instanceof BrickDestructibleState))
			return;
		BrickDestructibleState s = (BrickDestructibleState) brick.getState();
		s.addToLife(-1);
		if(s.getLife() == 0)
			s.processDeath(100);
	}
	
	@Override
	public void visit(ForceField forceField) {
	}

	@Override
	public void visit(Player player) {
	}

	@Override
	public void visit(Powerup powerup) {
	}

	@Override
	public void visit(Projectile projectile) {
	}
}

Most of the methods are empty because projectiles can only collide with bricks, but afaik, the visitor pattern pretty much requires you to provide arguments for each entity type.

In the entity itself, I simply call:

public void processCollision(Mob m) {
		accept(m.getVisitor());
	}

and the pattern does the rest.

#11

Here is an example Mob for further info on my implementation.

package com.noah.breakit.entity.mob.brick;

import com.noah.breakit.entity.mob.Mob;
import com.noah.breakit.entity.state.State;
import com.noah.breakit.graphics.Screen;
import com.noah.breakit.visitor.BrickVisitor;
import com.noah.breakit.visitor.Visitor;

public class Brick extends Mob {

	public Brick(int x, int y, State state) {
		super(x, y, state);
		width = 16;
		height = 8;
		visitor = new BrickVisitor().init(this);
	}
	
	@Override
	public void update() {
		state.update();
	}
	
	public void render(Screen screen) {
		state.render(screen);
	}
	
	public void processCollision(Mob m) {
		accept(m.getVisitor());
	}
	
	@Override
	public void accept(Visitor v) {
		v.visit(this);
	}
}

I’m also reading the post you provided; just now getting around to it. Thanks for the info.

#12

There’s a new trick in Java 8 which means you don’t need to provide empty method implementions in every implementing class: declare the visitor with default methods:


interface Visitor {
default void visit(Grobbit g) {}
default void visit(Squirgle s) {}
default void visit(Flimbo f) {}
}

Bingo, you no longer need to litter your implementations with empty methods any more.

Cas :slight_smile:

#13

IMO if instanceof is a wart, the visitor pattern is a tumour.

A) don’t be overly paranoid about instanceof Some usage isn’t bad. Working around the lack of multiple dispatch in Java by adding a load of boilerplate is often not worth it.

B) Java 8 is a brave new world. Check out approaches to functional pattern matching.

#14

Seriously, what is wrong with visitor? It’s fast, it works, it catches anything awry at compile time, and with default methods on interfaces it’s not even ugly any more - what’s not to like?

Cas :slight_smile:

#15

I don’t want to comment on the other topics (everyone has different preferences anyway), but such code is usually a sign that you are doing stuff in the wrong place.

All it does is modify the state of BrickDestructibleState, so try to do it in BrickDestructibleState, like:

// Visitor.visit() -> Brick.visit()
@Override
public void visit(Brick brick) {
	brick.getState().onHit();
}

// some interface for State, e.g. StateVisitor.onHit() -> BrickDestructibleState.onHit()
@Override
public void onHit() {
	addToLife(-1);
	if(getLife() == 0)
		processDeath(100);
}

If you continue to refine your code like this, in small steps, you will find code magically disappearing, and other being simplified. For example, your Visitor interface may melt down to one function “visit(Mob m)”.

#16

A lot! It breaks abstraction, adds a load of code and ends up with logic in the wrong (or less convenient) place. instanceof and the visitor pattern are both workarounds to Java’s lack of double-dispatch, the latter originating from C++ IIRC. Needing to use either can therefore be a sign that your code is structured wrongly. That doesn’t mean that you should never use either! A few instanceof statements may then actually perform equally well or better than the visitor pattern, not use 10x the code, and keep your logic in the right place.

I agree with both @CoDi^R and @philfrei - look at code restructuring and look at enums before using either instanceof or the visitor pattern. But also don’t listen to anyone saying that any use of instanceof is a code smell.

I like the simple (or less simple) approaches to pattern matching over the visitor pattern, because it doesn’t require changes to the target classes and keeps the logic together.

#17

Jeez, that’s among the most awful code I’ve ever had to look upon :frowning:

Here’s why and when you should use visitor:

  1. You know all the possible types in advance but you might add more later
  2. You have access to the types to change them have them all implement the acceptor
  3. You need it to be fast-as-fuck
  4. You want to write (and hence maintain) the bare minimum of actual code

This so happens to fit collision detection in games but there are lots of other cases where these four criteria are bang-on.

I tend to use instanceof only when I can’t apply a visitor because it’s not my code so I can’t just stuff an acceptor interface on it. I tend to shy away from switch statements on enums because when enums grow, switch statements tend to fall behind, whereas the visitor breaks immediately at compile time (without default interface implementations) or at least doesn’t actually fail.

IMO casting in OOP is one of the strongest bad smells in code, and instanceof is essentially a cast.

Cas :slight_smile:

#18

I agree with @princec. The code linked to by @nsigma is nothing but instanceof-in-disguise wrapped in Java 8 to make it less intelligible.

The reason why people “feel” that the use of instanceof “might” be bad, is because most of the time it is used because the model already breaks the Liskov substitution principle.
The always-excercised example is [icode]Square extends Rectangle[/icode]. In that case you will have to use [icode]if (rectangle instanceof Square) {…}[/icode] in some places because you have modelled your classes in such a way that you cannot easily substitute a Square for a Rectangle, because they have different semantic behaviours, which is implemented in their operations/methods.
The reason why using instanceof in that case is bad, is because you are coupling the behaviour and the constraints of your model to places other than the model classes.
And again: Changes become more difficult to incorporate.
I say it all the time: Think about how changes can be contained to as few places as possible. @princec named a few motivations for changes. And the visitor pattern is a good way to contain those changes to as few classes as possible (most likely just the visitor implementation itself).

#19

Welcome to our functional future! ;D

1-3 may be right (3 might depend on number of types), but 4?! That’s why I hate it.

So is breaking encapsulation, which is why I’d shy away from both whenever possible.

#20

But done in a less verbose, safer and compile-time-checkable way! It’s getting closer to smart casts in Kotlin (eg. https://kotlinlang.org/docs/reference/typecasts.html)

I generally agree with the bulk of what you’ve said by the way - this is always for edge cases.