Java-Gaming.org Hi !
Featured games (90)
games approved by the League of Dukes
Games in Showcase (711)
Games in Android Showcase (213)
games submitted by our members
Games in WIP (785)
games currently in development
News: Read the Java Gaming Resources, or peek at the official Java tutorials
 
    Home     Help   Search   Login   Register   
Pages: [1] 2
  ignore  |  Print  
  getting rid of "instanceof"  (Read 3622 times)
0 Members and 1 Guest are viewing this topic.
Offline DayTripperID

Senior Devvie


Medals: 8
Projects: 1
Exp: 1-3 months


Living is good!


« Posted 2017-01-10 18:51:40 »

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

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  
26  
27  
28  
29  
30  
31  
32  
33  
34  
35  
36  
37  
38  
39  
40  
41  
42  
43  
44  
45  
46  
47  
48  
49  
50  
51  
52  
53  
54  
55  
56  
57  
58  
59  
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!

Living is good!
Offline DayTripperID

Senior Devvie


Medals: 8
Projects: 1
Exp: 1-3 months


Living is good!


« Reply #1 - Posted 2017-01-10 19:20:04 »

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

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

Living is good!
Offline Archive
« Reply #2 - Posted 2017-01-10 20:03:58 »

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.

Games published by our own members! Check 'em out!
Legends of Yore - The Casual Retro Roguelike
Offline DayTripperID

Senior Devvie


Medals: 8
Projects: 1
Exp: 1-3 months


Living is good!


« Reply #3 - Posted 2017-01-10 20:43:52 »

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:
1  
2  
3  
4  
5  
6  
7  
8  
9  
10  
11  
12  
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:

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  
26  
27  
28  
29  
30  
31  
32  
33  
34  
35  
36  
37  
38  
39  
40  
41  
42  
43  
44  
45  
46  
47  
48  
49  
50  
51  
52  
53  
54  
55  
56  
57  
58  
59  
60  
61  
62  
63  
64  
65  
66  
67  
68  
69  
70  
71  
72  
73  
74  
75  
76  
77  
78  
79  
80  
81  
82  
83  
84  
85  
86  
87  
88  
89  
90  
91  
92  
93  
94  
95  
96  
97  
98  
99  
100  
101  
102  
103  
104  
105  
106  
107  
108  
109  
110  
111  
112  
113  
114  
115  
116  
117  
118  
119  
120  
121  
122  
123  
124  
125  
126  
127  
128  
129  
130  
131  
132  
133  
134  
135  
136  
137  
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.

Living is good!
Offline philfrei
« Reply #4 - Posted 2017-01-10 21:37:08 »

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

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

music and music apps: http://adonax.com
Offline princec

« JGO Spiffy Duke »


Medals: 895
Projects: 3
Exp: 16 years


Eh? Who? What? ... Me?


« Reply #5 - Posted 2017-01-10 23:23:38 »

See "Visitor Pattern".

Cas Smiley

Offline DayTripperID

Senior Devvie


Medals: 8
Projects: 1
Exp: 1-3 months


Living is good!


« Reply #6 - Posted 2017-01-10 23:41:05 »

@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!

Living is good!
Offline DayTripperID

Senior Devvie


Medals: 8
Projects: 1
Exp: 1-3 months


Living is good!


« Reply #7 - Posted 2017-01-11 16:08:55 »

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

Living is good!
Offline princec

« JGO Spiffy Duke »


Medals: 895
Projects: 3
Exp: 16 years


Eh? Who? What? ... Me?


« Reply #8 - Posted 2017-01-11 16:32:09 »

Check out this post from .... 2002.

http://www.java-gaming.org/topics/the-xap-development-diary/200/msg/1573/view.html#msg1573

Cas Smiley

Offline DayTripperID

Senior Devvie


Medals: 8
Projects: 1
Exp: 1-3 months


Living is good!


« Reply #9 - Posted 2017-01-11 19:19:02 »

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:

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  
26  
27  
28  
29  
30  
31  
32  
33  
34  
35  
36  
37  
38  
39  
40  
41  
42  
43  
44  
45  
46  
47  
48  
49  
50  
51  
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:

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


and the pattern does the rest.

Living is good!
Games published by our own members! Check 'em out!
Legends of Yore - The Casual Retro Roguelike
Offline DayTripperID

Senior Devvie


Medals: 8
Projects: 1
Exp: 1-3 months


Living is good!


« Reply #10 - Posted 2017-01-11 19:28:16 »

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

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  
26  
27  
28  
29  
30  
31  
32  
33  
34  
35  
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.

Living is good!
Offline princec

« JGO Spiffy Duke »


Medals: 895
Projects: 3
Exp: 16 years


Eh? Who? What? ... Me?


« Reply #11 - Posted 2017-01-11 19:37:24 »

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:
1  
2  
3  
4  
5  
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 Smiley

Offline nsigma
« Reply #12 - Posted 2017-01-11 22:54:41 »

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.

Praxis LIVE - hybrid visual IDE for (live) creative coding
Offline princec

« JGO Spiffy Duke »


Medals: 895
Projects: 3
Exp: 16 years


Eh? Who? What? ... Me?


« Reply #13 - Posted 2017-01-12 00:01:50 »

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 Smiley

Offline CoDi^R
« Reply #14 - Posted 2017-01-12 08:14:57 »

1  
2  
3  
4  
5  
6  
7  
8  
9  
10  
@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);
   }
}


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:

1  
2  
3  
4  
5  
6  
7  
8  
9  
10  
11  
12  
13  
// 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)".

Robotality - steamworks4j - @code_disaster - codi^r @ #java-gaming
Offline nsigma
« Reply #15 - Posted 2017-01-12 12:12:29 »

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?

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.

Praxis LIVE - hybrid visual IDE for (live) creative coding
Offline princec

« JGO Spiffy Duke »


Medals: 895
Projects: 3
Exp: 16 years


Eh? Who? What? ... Me?


« Reply #16 - Posted 2017-01-12 12:28:05 »

Jeez, that's among the most awful code I've ever had to look upon Sad

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-f**k
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 Smiley

Offline KaiHH

JGO Kernel


Medals: 384



« Reply #17 - Posted 2017-01-12 12:51:44 »

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
Square extends Rectangle
. In that case you will have to use
if (rectangle instanceof Square) {...}
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).
Offline nsigma
« Reply #18 - Posted 2017-01-12 12:51:58 »

Jeez, that's among the most awful code I've ever had to look upon Sad

Welcome to our functional future!  Grin

4. You want to write (and hence maintain) the bare minimum of actual code

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

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

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

Praxis LIVE - hybrid visual IDE for (live) creative coding
Offline nsigma
« Reply #19 - Posted 2017-01-12 13:03: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.

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.

Praxis LIVE - hybrid visual IDE for (live) creative coding
Offline 65K
« Reply #20 - Posted 2017-01-12 13:49:39 »


I like the simple
That's an example why the Java universe is famous for its love for over engineering.
Keep it simple, readable and maintainable.

Lethal Running - a RPG about a deadly game show held in a futuristic dysoptian society.
Offline princec

« JGO Spiffy Duke »


Medals: 895
Projects: 3
Exp: 16 years


Eh? Who? What? ... Me?


« Reply #21 - Posted 2017-01-12 15:00:39 »

So is breaking encapsulation, which is why I'd shy away from both whenever possible.
I don't get the "breaking encapsulation" part here.

Cas Smiley

Offline princec

« JGO Spiffy Duke »


Medals: 895
Projects: 3
Exp: 16 years


Eh? Who? What? ... Me?


« Reply #22 - Posted 2017-01-12 15:31:18 »

Have a look at these three different ways of doing the same thing. Each has its nuances.

Firstly, the visitor pattern. The visitor pattern has these advantages:
  • Nearly the fastest implementation
  • Gracefully handles sub-hierarchies
  • To implement new effects, we simply implement different EntityVisitors that act upon the concrete subclasses directly
  • If we want to ensure that something new is always explicitly handled we don't have to use a default implementation and the compiler will complain
and these disadvantages:
  • To implement new Entities classes, we must be able to also change the EntityVisitor interface
  • Requires instantiation of an EntityVisitor instance, though this is likely escaped, and likely cacheable

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  
26  
27  
28  
29  
30  
31  
32  
33  
34  
35  
36  
37  
38  
39  
40  
interface EntityVisitor {
   default void visit(Monster m) {}
   default void visit(FriendlyMonster m) {}
   default void visit(Player p) {}
   default void visit(Arrow a) {}
}

abstract class Entity {
   abstract void accept(EntityVisitor v);
}

class Monster extends Entity {
   void accept(EntityVisitor v) { v.visit(this); }
   void kill() {...}
}
class FriendlyMonster extends Monster {
   @Override
   void accept(EntityVisitor v) { v.visit(this); }
}
class Player extends Entity {
   void accept(EntityVisitor v) { v.visit(this); }
   void damage(int points) {...}
}
class Arrow extends Entity {
   void accept(EntityVisitor v) { v.visit(this); }
   void remove() {...}
}

...
// Poison gas cloud! Kill all monsters, and damage all players. Arrows are unaffected
entities.forEach(e -> e.accept(new EntityVisitor() {
   void visit(Monster m) { m.kill(); }
   void visit(Player p) { p.damage(1); }
});

// Smartbomb! Kill all monsters, remove arrows
entities.forEach(e -> e.accept(new EntityVisitor() {
   void visit(Monster m) { m.kill(); }
   void visit(Arrow a) { a.remove(); }
});


Here is what it looks like with an enum type. It has the following advantages:
  • Very easy to understand what it does at a glance
  • Entity can change what it returns from getType(), allowing it to "change" its class effectively. Not sure if this is an advantage in this case Wink

.. and disadvantages:
  • Not as fast as visitor: requires a virtual method call, then a switch, then a cast just to get to where we want to send messages to the right thing
  • If we want to add a new Entity class, we must be able to change EntityType to add a new enum for it
  • There is no actual connection between EntityType and the subclasses of Entity - it simply relies on the cast.

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  
26  
27  
28  
29  
30  
31  
32  
33  
34  
35  
36  
37  
38  
39  
40  
41  
42  
43  
44  
enum EntityType { MONSTER, FRIENDLY_MONSTER, PLAYER, ARROW }
abstract class Entity {
   abstract EntityType getType();
}

class Monster extends Entity {
   EntityType getType() { return EntityType.MONSTER; }
   void kill() {...}
}
class FriendlyMonster extends Monster {
   @Override
   EntityType getType() { return EntityType.FRIENDLY_MONSTER; }
}
class Player extends Entity {
   EntityType getType() { return EntityType.PLAYER; }
   void damage(int points) {...}
}
class Arrow extends Entity {
   EntityType getType() { return EntityType.ARROW; }
   void remove() {...}
}
// Poison gas cloud! Kill all monsters, and damage all players. Arrows are unaffected
entities.forEach(e -> {
   switch (e.getType()) {
      case MONSTER:
         ((Monster) e).kill();
         break;
      case PLAYER:
         ((Player) e).damage(1);
         break;
   }
});

// Smartbomb! Kill all monsters, remove arrows
entities.forEach(e -> {
   switch (e.getType()) {
      case MONSTER:
         ((Monster) e).kill();
         break;
      case ARROW:
         ((Arrow) e).remove();
         break;
   }
});



And with instanceof. The advantages being:
  • Classes no longer need to be in an actual hierarchy or implement some common interface
and disadvantages being:
  • How do we differentiate between Monster and FriendlyMonster? We have to remember to check for the subtype before we check the super type... get it wrong and wierd things happen to your FriendlyMonsters which are strangely treated like Monsters. Imagine if you decided to change the type hierarchy so that FriendlyMonster inherited instead from Player... or Entity.
  • Slowest implementation
  • Ugly cast only gets uglier if you want to call more than one method

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  
26  
27  
28  
29  
30  
31  
32  
class Monster {
   void kill() {...}
}
class FriendlyMonster {
}
class Player {
   void damage(int points) {...}
}
class Arrow {
   void remove() {...}
}
// Poison gas cloud! Kill all monsters, and damage all players. Arrows are unaffected
entities.forEach(e -> {
   if (e instanceof FriendlyMonster) {
      // Don't do anything to FriendlyMonster!
   } else if (e instanceof Monster) {
      ((Monster) m).kill();
   } else if (e instanceof Player) {
      ((Player) e).damage(1);
   }
});

// Smartbomb! Kill all monsters, remove arrows
entities.forEach(e -> {
   if (e instanceof FriendlyMonster) {
      // Don't do anything to FriendlyMonster!
   } else if (e instanceof Monster) {
      ((Monster) e).kill();
   } else if (e instanceof Arrow) {
      ((Arrow) e).remove();
   }
});


And on a related but important tangent, another way to implement all that using polymorphic dispatch. This is great when you know in advance what effects you might be hitting the Entities with.
Advantages:
  • Absolute fastest possible implementation
  • Super easy to see what's going on

Disadvantages:
  • We must know every effect that can occur to the Entities up front, rather than creating new effects by building them up from smaller effects - we cannot add new effects if we don't have access to Entity source
  • Once you've got a lot of effects, your base class becomes huge, full of empty implementations

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  
26  
27  
28  
29  
30  
31  
32  
33  
34  
abstract class Entity {
   void onPoisonGas() {}
   void onSmartbomb() {}
}

class Monster extends Entity {
   @Override
   void onPoisonGas() { kill(); }
   @Override
   void onSmartBomb() { kill(); }
   void kill() {...}
}
class FriendlyMonster extends Entity {
   @Override
   void onPoisonGas() { // Don't do anything! }
   @Override
   void onSmartBomb() { // Don't do anything! }
   void kill() {...}
}
class Player extends Entity {
   @Override
   void onPoisonGas() { damage(1); }
   void damage(int points) {...}
}
class Arrow extends Entity {
   @Override
   void onSmartBomb() { remove(); }
   void remove() {...}
}
// Poison gas cloud! Kill all monsters, and damage all players. Arrows are unaffected
entities.forEach(Entity::onPoisonGas);

// Smartbomb! Kill all monsters, remove arrows
entities.forEach(Entity::onSmartBomb);


So... just some comparisons to think about.

Cas Smiley

Offline nsigma
« Reply #23 - Posted 2017-01-12 16:45:42 »

I don't get the "breaking encapsulation" part here.

Well, from GoF -

Quote
Visitor's approach assumes that the ConcreteElement interface is powerful enough to let visitors do their job. As a result, the pattern often forces you to provide public operations that access an element's internal state, which may compromise its encapsulation.

In general of your examples I prefer the behaviour in the entities (your last example), but they all have their places. Also, instanceof is not likely to be the slowest - enum would probably be slower (have seen a benchmark of that somewhere). Incidentally, the pattern matching style approach to your instanceof example could be something along the lines of -

1  
2  
3  
entities.forEach(e -> when(e)
        .is(Monster.class, m -> m.kill())
        .is(Player.class, this::updatePlayer);

Praxis LIVE - hybrid visual IDE for (live) creative coding
Offline philfrei
« Reply #24 - Posted 2017-01-12 20:27:29 »

Thanks to folks to contributing to this discussion. I am very much looking forward to examining the discussion points in detail.

It seems to me, on general principles, if a subclass doesn't allow you to use the same methods of the parent class, differences being accounted for via Override, then something is fishy about the decision to subclass in that instance.

I will have to dive into the details to figure out why collision-testing is not something where one can simply make an Interface, e.g., Collidable or OccupiesSpace, and cover the needed object requirements in the various implementations of the method, so that the same method call can suffice. Seems to me like it should be possible. But, I haven't had to do collision testing in my game or audio programs, and the alternative algorithm that I tested (mapping the object to a grid similar in size to the actual screen, and testing the points you traverse or move to for occupants before occupying them), worked pretty well.

music and music apps: http://adonax.com
Offline Sickan
« Reply #25 - Posted 2017-01-12 21:30:30 »

@princec that's a very interesting thread, thank you for sharing.

Personally I would make an abstract method and call that, then implement the behavior in the subclasses/shared superclass if they have the same behavior. But if I don't feel like being meticulous I'll just use an instanceof.
Offline ShannonSmith
« Reply #26 - Posted 2017-01-12 21:54:00 »

I went off anything described as a design pattern in a big way after working at a company where they were considered sacrosanct.
Easily the most painful code I have ever had to work on, approaching http://discuss.joelonsoftware.com/?joel.3.219431.12 levels of bad.
It is probably an overreaction to a single example of overuse of design patterns, but it left such a bad taste in my mouth that I remain wary to this day.

My personal opinion is that the advantage:

Quote
Super easy to see what's going on

Pretty much trumps everything else, until you need to optimize for performance with a profiler to prove where your bottleneck is.
An enum type member variable is what works for me these days.
Offline 65K
« Reply #27 - Posted 2017-01-13 07:28:34 »

Thing with design patterns is that everybody uses them, knowingly or not. Patterns aren't even restricted to software development. Humans love patterns.
Nobody ever recommended to build software with a pattern cheat sheet on your desk.

Lethal Running - a RPG about a deadly game show held in a futuristic dysoptian society.
Offline kevglass

« JGO Spiffy Duke »


Medals: 319
Projects: 25
Exp: 22 years


Coder, Trainee Pixel Artist, Game Reviewer


« Reply #28 - Posted 2017-01-13 08:43:06 »

Quote
Nobody ever recommended to build software with a pattern cheat sheet on your desk.

Thats the problem, some people do, lets apply it everywhere man!

Cheers,

Kev

Offline cylab

JGO Kernel


Medals: 162



« Reply #29 - Posted 2017-01-13 09:51:58 »

I don't get the "breaking encapsulation" part here.

Well, from GoF -

Quote
Visitor's approach assumes that the ConcreteElement interface is powerful enough to let visitors do their job. As a result, the pattern often forces you to provide public operations that access an element's internal state, which may compromise its encapsulation.

This made me think, if a more component like approach using a heterogeneous container would be desirable here:

Define what actually can happen in your game as interfaces
1  
2  
3  
4  
5  
6  
7  
8  
9  
10  
@FunctionalInterface
interface PoisonGasEffect {
    // this way, you can even require signatures for your effects
    void onPoison(int toxicity);
}

@FunctionalInterface
interface SmartBombEffect {
    void onSmartBomb();
}


Putting a registy in your Entity baseclass would allow for using composition to model the entities behaviour
1  
2  
3  
4  
5  
6  
7  
8  
9  
10  
11  
12  
13  
14  
15  
16  
abstract class Entity {
    private Map<Class<?>, Object> handlers = new HashMap<>();

    protected <T> void register(Class<T> type, T instance) {
        handlers.put(type, instance);
    }

    public <T> boolean supports(Class<T> type) {
        return handlers.containsKey(type);
    }

    @SuppressWarnings("unchecked")
    public <T> T getHandler(Class<T> type) {
        return (T) handlers.get(type);
    }
}


Using Java 8, you can define the effects on your Entities with relative ease:
1  
2  
3  
4  
5  
6  
7  
8  
9  
10  
11  
12  
13  
14  
15  
16  
17  
18  
19  
20  
21  
22  
class Monster extends Entity {
    public Monster() {
        register(SmartBombEffect.class, () -> kill());
        register(PoisonGasEffect.class, (toxicity) -> kill());
    }
    void kill() { /* ... */ }
}

class FriendlyMonster extends Monster {
    public FriendlyMonster() {
        register(SmartBombEffect.class, () -> {}); // this basically "overrides" the effect in the _unfriendly_ Monster
        register(PoisonGasEffect.class, (toxicity) -> damage(toxicity));
    }
    void damage(int points) { /* ... */ }
}

class Arrow extends Entity {
    public Arrow () {
        register(SmartBombEffect.class, () -> remove());
    }
    void remove() { /* ... */ }
}


This works also in a more traditional way:
1  
2  
3  
4  
5  
6  
7  
8  
9  
10  
11  
12  
13  
14  
15  
16  
17  
18  
19  
20  
21  
22  
class Player extends Entity {

    // or if you want to...
    private class MyUberHandler implements SmartBombEffect, PoisonGasEffect {

        public void onPoison(int toxicity) {
            damage(toxicity);
        }

        public void onSmartBomb() {
            System.out.println("HAHAHA - I told you: 'DONT MESS WITH ME!!!!'");
        }
    }

    public Player () {
        MyUberHandler handler = new MyUberHandler();
        register(SmartBombEffect.class, handler);
        register(PoisonGasEffect.class, handler);
    }

    void damage(int points) { /* ... */ }
}


The actual gameloop would then be rid of specific implementations for your Entity - it's up to you, if you consider this a good or a bad thing...
1  
2  
3  
4  
5  
6  
7  
8  
9  
10  
11  
12  
// ...
// Poison gas cloud! The effect is encapsulated in the Entity implementation
entities.forEach(e -> {
    if(e.supports(PoisonGasEffect.class))
        e.getHandler(PoisonGasEffect.class).onPoison(1);
});

// Smartbomb! Whatever happens will happen...
entities.forEach(e -> {
    if(e.supports(SmartBombEffect.class))
        e.getHandler(SmartBombEffect.class).onSmartBomb();
});

Mathias - I Know What [you] Did Last Summer!
Pages: [1] 2
  ignore  |  Print  
 
 

 
numerical (172 views)
2017-02-21 07:32:16

numerical (172 views)
2017-02-21 07:31:46

theagentd (280 views)
2017-02-18 13:42:33

theagentd (280 views)
2017-02-18 13:35:16

h.pernpeintner (1447 views)
2017-01-24 22:39:11

h.pernpeintner (1432 views)
2017-01-24 22:38:32

Galdo (1996 views)
2017-01-12 13:44:09

Archive (2046 views)
2017-01-02 05:31:41

0AndrewShepherd0 (2583 views)
2016-12-16 03:58:39

0AndrewShepherd0 (2332 views)
2016-12-15 21:50:57
List of Learning Resources
by elect
2016-09-09 09:47:55

List of Learning Resources
by elect
2016-09-08 09:47:20

List of Learning Resources
by elect
2016-09-08 09:46:51

List of Learning Resources
by elect
2016-09-08 09:46:27

List of Learning Resources
by elect
2016-09-08 09:45:41

List of Learning Resources
by elect
2016-09-08 08:39:20

List of Learning Resources
by elect
2016-09-08 08:38:19

Rendering resources
by Roquen
2016-08-08 05:55:21
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!