(instanceof) Is it EVER ok?

Newbie & Debugging Questions
#1

Long story short, I’m making a game. I have a Level class that contains entities called Interactables that implement the game logic. Problem is, I’ve made many extentions to the Interactable class. Moveable, Controllable, Collider, Collidable, etc. When I add an interactable to the game, I write something like…


public void addInteractable(Interactable inter)
{
    interactables.add(inter);

    if(inter instanceof Moveable)
        moveables.add((Moveable)inter);

    if(inter instanceof Controllable)
        controllables.add((Controllable)inter);

    if(inter instanceof Collider)
        colliders.add((Collider)inter);

    if(inter instanceof Collidable)
        collidables.add((Collidable)inter);
}

The reason I split the Interactable into multiple lists is to reduce the number of comparisons between Interactables. For instance, Colliders only need to be concerned with Collidables, so when applying collisions, I iterate through colliders and collidables enforcing collisions. The split is also necessary because certain routines belonging to certain Interactables need to be called in a particular order. Collidables need to move before Colliders enforce collisions.

Of course you could imagine that removing an Interactable from the level would be a similar process as to the one above, so I’m not going to write that out.

My real question is, is this a bad practice? Would someone else consider a different approach as to adding and removing entities from a level? I know instanceof checks are a sign of bad design, but I felt that they had their place in this one area of my game.

#2

Other than a variable inside ‘inter’ that tells you what type it is, there isn’t much else you could do so I think you’re fine. You would want to use an if-else chain for performance reasons though :wink:

#3

Can’t always do if-else since I use a lot of interfaces (I do in a couple of instances, though) Good to know that I won’t be shunned by the Java community for doing this.

#4

IMO designing your API around instanceof is a poor practice.

Your code looks like a classic example of over-engineering. It adds a lot of complexity and potential for bugs (e.g. entities not being in their expected order since they are contained in different lists) with very little gain.

I have two suggestions:

  1. Use a single list for all of your game entities. Each entity is a subclass of the Entity class (or interface) which contains some basic stuff that is shared by most entities, e.g.
void update(int);
void setPosition(int x, int y);
void onCollide(Entity e);

So what if your “Tree” entity has an update method even if the sprite doesn’t have any logic? So what if your “FloatyParticle” entity will never collide with another entity, and thus onCollide is made useless?

In the long run, it will be much easier to work with than several lists and frequent casts / instanceof checks. Chances are the game you’re working on isn’t going to be the size of World of Warcraft, and so you don’t need 100% scalable code.

  1. Use an entity component system like Artemis.
#5

Can’t really do that. I NEED seperate lists to cut down on the number of comparisons. There could be possibly THOUSANDS of objects in my game that need to compare themselves to each other. Let me give you an example.

At first, I made all of my Collidable objects check for collision with other Collidable objects. This was fine until I reached the hundreds. Enforcing collision isn’t the cheapest thing in the world, and when 400 Collidable objects had to compare themselves to each other, my game’s framerate dropped down to a meager 10fps when the game was supposed to run at 50fps. After that, I split Collidables with Colliders and forced them into seperate lists. Brought the speed up back to it’s original buttery smooth 50fps. This also guaranteed that certain operations ran in a particular order, so less glitches.

#6

What calls addInteractable()?

I suspect a better solution could be something like having separate addMovable(Movable) addColliidable(Collidable) addCollider(Collider) etc., and then calling each one as appropriate from the source object.

eg.


class Foo implements Movable, Collidable, Interactable
{
  public void register(Context context)
  {
    context.addMovable(this);
    context.addCollidable(this);
    context.addInteractable(this);
  }
}

Then each object registers itself for the correct subsystems it depends on.

Answering the more generic question ‘is instanceof ok?’, I’d say yes. But it (usually) implies that you’ve actually got some other bit of your design wrong somewhere. And it tends to be a bit of a maintenance nightmare long term.

#7

Yeah, but that would force anyone implementing my code to be sure to add the object to the correct list. If Collidable extends Moveable, and I simply try to add the Collidable to the Moveable list, it will only be treated as a Moveable. I’m really not concerned with the efficiency of the add and remove methods because they are not constantly called. Ugh, this is still tough. I need to isolate the Entities to an extent, but this kinda makes my code a little inflexible. I feel like the Entities should be responsible for what happens, and the Level should be responsible for when. I’m not proud of the instance-of checks, but I really can’t think of a more simplistic solution. :frowning:

#8

I have heard instanceof is surprisingly slow and it’s one those things that has a bad code smell. However, its a safe, well supported feature, so it won’t hurt anything if used in exceptional cases. And you could mimic it with a variable like ra4king suggested if it becomes a bottleneck. In your case, I would not dwell on instanceof since it seems like you can confine it to that one method and eliminate extra executions of instanceof.

Edit: Two new posts while writing this. I thought of that and came to similar conclusions. I think confining instanceof to your add method so you can avoid it everywhere else is actually better OOP.

#9

Personally I don’t see that as being a big issue, so we might have to disagree on that.

More practically, your composite addInteractable with instanceof is bad in terms of code orthogonality. You’re forcing all objects of Movable (etc.) to also be Interactable, which I don’t think is necessary. And you’re forcing all of them to be the same object, and all added/removed at the same time.

For example, you might have a game object like, say, a controllable car. Most of the time it’s movable, sometimes it’s controllable, and sometimes it’s collidable. By moving the add/remove into separate functions the object can take responsibility for exactly when in it’s lifetime it is collidable/controllable/etc.

#10

Well well well! I never considered something like that! As long people don’t have a problem with the fact that I’m adding objects to multiple lists, I may choose to implement your idea in the future. I still need all Entities to have the lowest common denominator, Interactable, but I can see where adding objects to multiple lists can be helpful.

#11

Use of instanceof is sometimes unavoidable in even a sane API, but usually it’s because you’re dealing with some data producer that “flattens” everything into a base class, possibly all the way up to Object. Network deserialization is a big culprit there. Other APIs will also force you to check types, such as a message-passing library that passes everything as subclasses of Message. The handler logic for messages isn’t polymorphic, it doesn’t live on Message, so you’re forced to use instanceof checks.

It’s still a very rank code smell, because you’re using it to enforce a constraint at runtime that you should be expressing statically at compile time. It’s really the worst of both worlds between static and duck typing. With duck typed languages, you at least get something like structural typing: just call .quack(), and if it can quack, it will.

So the moral of the story is, use the type system as much as possible, and if you do have to work around it, make sure you know exactly why, and that you’re not just doing it out of laziness.

#12

In my current design, when I want to remove an Interactable, I call removeInteractable from the level. What this does is flags whatever Interactable needs to be removed, then removes the Interactable at a safe time. Since I’m going to have multiple add methods, I’m pretty much going to have to throw away this method of object removal. I think it was you, Orangy Tang, that pointed me to the Bag class made by kappa. It was in that thread I made about ArrayList vs Linked list. Ya know, the thread that wouldn’t die? I found out that while iterating through this collection backwards, it was safe to remove objects from it. I just found this little piece of information interesting enough to post.

#13

most instanceofs can replaceced with polymorphism.


class Interactable{
   ...
   public void  addToSpezialCollection(Collection moveables, Collection controllables, Collection colliders, Collection collidables);
   public void  removeFromSpezialCollection(Collection moveables, Collection controllables, Collection colliders, Collection collidables);
}

Moveable extends Interactable{
  public void  addToSpezialCollection(Collection moveables, Collection controllables, Collection colliders, Collection collidables){
    moveables.add(this);
  }

  public void  removeFromSpezialCollection(Collection moveables, Collection controllables, Collection colliders, Collection collidables){
    moveables.remove(this);
  }
}

Controllable extends Interactable{
  public void  addToSpezialCollection(Collection moveables, Collection controllables, Collection colliders, Collection collidables){
    controllables.add(this);
  }

  public void  removeFromSpezialCollection(Collection moveables, Collection controllables, Collection colliders, Collection collidables){
    controllables.remove(this);
  }
}



public void addInteractable(Interactable inter){
    interactables.add(inter);

    inter.addToSpezialCollection(moveables,controllables,colliders,collidables);
}
#14

This used to be the case in an older version of Java - 1.5, iirc. Java 6 and on, it’s not a concern performance-wise.