Java-Gaming.org    
Featured games (79)
games approved by the League of Dukes
Games in Showcase (475)
Games in Android Showcase (106)
games submitted by our members
Games in WIP (529)
games currently in development
News: Read the Java Gaming Resources, or peek at the official Java tutorials
 
    Home     Help   Search   Login   Register   
Pages: [1]
  ignore  |  Print  
  (instanceof) Is it EVER ok?  (Read 2221 times)
0 Members and 1 Guest are viewing this topic.
Offline DrewLols

Senior Member


Medals: 1
Projects: 1


Noob going through metamorphosis...


« Posted 2012-08-29 22:34:49 »

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

1  
2  
3  
4  
5  
6  
7  
8  
9  
10  
11  
12  
13  
14  
15  
16  
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.

Did you know that 90% of statistics are wrong?
Offline ra4king

JGO Kernel


Medals: 336
Projects: 2
Exp: 5 years


I'm the King!


« Reply #1 - Posted 2012-08-29 22:43:31 »

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

Offline DrewLols

Senior Member


Medals: 1
Projects: 1


Noob going through metamorphosis...


« Reply #2 - Posted 2012-08-29 22:54:28 »

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.

Did you know that 90% of statistics are wrong?
Games published by our own members! Check 'em out!
Legends of Yore - The Casual Retro Roguelike
Offline davedes
« Reply #3 - Posted 2012-08-29 23:14:40 »

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.
1  
2  
3  
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.

2. Use an entity component system like Artemis.

Offline DrewLols

Senior Member


Medals: 1
Projects: 1


Noob going through metamorphosis...


« Reply #4 - Posted 2012-08-29 23:23:45 »

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.

Did you know that 90% of statistics are wrong?
Online Orangy Tang

JGO Kernel


Medals: 56
Projects: 11


Monkey for a head


« Reply #5 - Posted 2012-08-29 23:38:40 »

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.

1  
2  
3  
4  
5  
6  
7  
8  
9  
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.

[ TriangularPixels.com - Play Growth Spurt, Rescue Squad and Snowman Village ] [ Rebirth - game resource library ]
Offline DrewLols

Senior Member


Medals: 1
Projects: 1


Noob going through metamorphosis...


« Reply #6 - Posted 2012-08-29 23:45:54 »

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.

1  
2  
3  
4  
5  
6  
7  
8  
9  
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.

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

Did you know that 90% of statistics are wrong?
Offline Best Username Ever

Junior Member





« Reply #7 - Posted 2012-08-29 23:57:16 »

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.
Online Orangy Tang

JGO Kernel


Medals: 56
Projects: 11


Monkey for a head


« Reply #8 - Posted 2012-08-30 00:04:55 »

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.

[ TriangularPixels.com - Play Growth Spurt, Rescue Squad and Snowman Village ] [ Rebirth - game resource library ]
Offline DrewLols

Senior Member


Medals: 1
Projects: 1


Noob going through metamorphosis...


« Reply #9 - Posted 2012-08-30 00:18:43 »

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.

Did you know that 90% of statistics are wrong?
Games published by our own members! Check 'em out!
Legends of Yore - The Casual Retro Roguelike
Offline sproingie

JGO Kernel


Medals: 201



« Reply #10 - Posted 2012-08-30 00:19:44 »

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.

Offline DrewLols

Senior Member


Medals: 1
Projects: 1


Noob going through metamorphosis...


« Reply #11 - Posted 2012-08-30 00:41:37 »

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.

Did you know that 90% of statistics are wrong?
Offline OttoMeier

Senior Member


Medals: 4
Projects: 1



« Reply #12 - Posted 2012-08-30 20:09:27 »

most instanceofs can replaceced with polymorphism.

1  
2  
3  
4  
5  
6  
7  
8  
9  
10  
11  
12  
13  
14  
15  
16  
17  
18  
19  
20  
21  
22  
23  
24  
25  
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);
  }
}

1  
2  
3  
4  
5  
public void addInteractable(Interactable inter){
    interactables.add(inter);

    inter.addToSpezialCollection(moveables,controllables,colliders,collidables);
}
Offline avm1979
« Reply #13 - Posted 2012-08-30 21:21:45 »

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.

Pages: [1]
  ignore  |  Print  
 
 
You cannot reply to this message, because it is very, very old.

 

Add your game by posting it in the WIP section,
or publish it in Showcase.

The first screenshot will be displayed as a thumbnail.

ctomni231 (31 views)
2014-07-18 06:55:21

Zero Volt (27 views)
2014-07-17 23:47:54

danieldean (23 views)
2014-07-17 23:41:23

MustardPeter (24 views)
2014-07-16 23:30:00

Cero (39 views)
2014-07-16 00:42:17

Riven (41 views)
2014-07-14 18:02:53

OpenGLShaders (28 views)
2014-07-14 16:23:47

Riven (28 views)
2014-07-14 11:51:35

quew8 (25 views)
2014-07-13 13:57:52

SHC (61 views)
2014-07-12 17:50:04
HotSpot Options
by dleskov
2014-07-08 03:59:08

Java and Game Development Tutorials
by SwordsMiner
2014-06-14 00:58:24

Java and Game Development Tutorials
by SwordsMiner
2014-06-14 00:47:22

How do I start Java Game Development?
by ra4king
2014-05-17 11:13:37

HotSpot Options
by Roquen
2014-05-15 09:59:54

HotSpot Options
by Roquen
2014-05-06 15:03:10

Escape Analysis
by Roquen
2014-04-29 22:16:43

Experimental Toys
by Roquen
2014-04-28 13:24:22
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
Powered by MySQL Powered by PHP Powered by SMF 1.1.18 | SMF © 2013, Simple Machines | Managed by Enhanced Four Valid XHTML 1.0! Valid CSS!