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...

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.

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.

1 2 3

voidupdate(int);voidsetPosition(intx, inty);voidonCollide(Entitye);

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.

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.

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.

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.

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.

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

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.

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.

Yeah, but that would force anyone implementing my code to be sure to add the object to the correct list.

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.

Yeah, but that would force anyone implementing my code to be sure to add the object to the correct list.

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.

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.

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.

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.

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.

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.

java-gaming.org is not responsible for the content posted by its members, including references to external websites,
and other references that may or may not have a relation with our primarily
gaming and game production oriented community.
inquiries and complaints can be sent via email to the info‑account of the
company managing the website of java‑gaming.org