That’s an example why the Java universe is famous for its love for over engineering.
Keep it simple, readable and maintainable.
I don’t get the “breaking encapsulation” part here.
Cas 
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
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
… 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.
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
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
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
Well, from GoF -
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 -
entities.forEach(e -> when(e)
.is(Monster.class, m -> m.kill())
.is(Player.class, this::updatePlayer);
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.
@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.
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:
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.
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.
[quote]Nobody ever recommended to build software with a pattern cheat sheet on your desk.
[/quote]
Thats the problem, some people do, lets apply it everywhere man!
Cheers,
Kev
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
@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
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:
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:
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…
// ...
// 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();
});
@cylab - I use that “pattern” a lot, but then it’s basically the same as the Lookup mechanism in the NetBeans platform. Not sure if it’s the fastest approach, but it’s definitely one of the nicer ones.
Incidentally, getHandler() could also return Optional to not require the supports() method, or you could even pass a consumer into a utility method on entity that does that for you. Hey, I like keeping the logic code succinct and less error prone. ;D
entities.forEach(e -> e.getHandler(PoisonGasEffect.class).ifPresent(h -> h.onPoison(1)));
entities.forEach(e -> e.with(PoisonGasEffect.class, h -> h.onPoison(1)));
Yupp. Can’t deny my Netbeans RCP past…
For the sake of completeness, this is, how Entity would look like, then:
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);
}
// support java 8 style optional lambda execution
public <T> void with(Class<T> type, Consumer<T> consumer) {
T handler = type.cast(handlers.get(type))
if(handler != null)
consumer.accept(handler);
}
}
Hey guys,
I just checked this post for the first time in a couple days, and I see there is a lot more to read. =) I did a lot more refactoring. Actually I have been rebuilding the game from the ground up, which has turned out to be the best thing I could have done. One of the things I did in thee process is move the Visitor implementation out of the Mob class and into the MobState class:
package breakit.mob.state;
import breakit.mob.Mob;
import breakit.visitor.Visitable;
import breakit.visitor.Visitor;
import engine.graphics.Screen;
public interface MobState extends Visitable, Visitor {
public MobState init(Mob m);
public MobState update();
public void render(Screen screen);
public void processCollision(Mob m);
public Mob getMob();
public void setMob(Mob m);
public Visitor getVisitor();
}
This solved the “instanceof” dilemma immediately. Now when I process a collision it is more like this:
public void visit(BallStateNormal visitable) {
Ball ball = (Ball) visitable.getMob();
brick.bounceBall(ball);
ball.addMultiplier(1);
multiplier = ball.getMultiplier();
}
Here is the state class that contains that visitor method for total context:
package breakit.mob.brick;
import breakit.gamestate.World;
import breakit.mob.Mob;
import breakit.mob.ball.Ball;
import breakit.mob.ball.BallStateNormal;
import breakit.mob.state.AbstractMobState;
import breakit.mob.state.MobState;
import breakit.visitor.Visitor;
import engine.graphics.Screen;
public class BrickStateDestructible extends AbstractMobState {
private Brick brick = null;
private int multiplier = 0;
@Override
public MobState init(Mob m) {
brick = (Brick) m;
brick.setCol(0xffff00);
return this;
}
@Override
public MobState update() {
if(brick.getLife() == 0) {
Integer points = multiplier * 100;
World w = (World) brick.getGameState();
w.addParticleSpawner((int)brick.getx(), (int)brick.gety(), 100);
w.addFloatingText((int)brick.getx(), (int)brick.gety() + 1, points.toString());
brick.remove();
}
return this;
}
@Override
public void render(Screen screen) {
screen.drawRect((int)brick.getx(), (int)brick.gety(), brick.getWidth(), brick.getHeight(), brick.getCol());
}
@Override
public void processCollision(Mob m) {
accept(m.getMobState().getVisitor());
}
@Override
public Mob getMob() {
return brick;
}
@Override
public void setMob(Mob m) {
brick = (Brick) m;
}
public void visit(BallStateNormal visitable) { //this method here
Ball ball = (Ball) visitable.getMob();
brick.bounceBall(ball);
ball.addMultiplier(1);
multiplier = ball.getMultiplier();
}
@Override
public void accept(Visitor visitor) {
visitor.visit(this);
}
}
So instead of each visit needing to check which state the Mob is in and respond accordingly, it now knows exactly what state it is dealing with and can just act.
Right now I have Mob classes which represent an game entity. The Mob references a separate state class that can be discarded and replaced by a newly created state on the fly. Brick is a bad example because it pretty much holds the same state for its lifetime. A brick is destructible or it is indestructible. It never changes state, although technically it could.
Ball, on the other hand, can change states between Normal, Power, and Giant on the fly. So really, the Mob just becomes a container for the fields such as x, y, xa, ya, color, etc. All game logic and rendering is done in the state classes, including the collision visits. Each state handles the game logic and collision uniquely:
package breakit.mob.ball;
import breakit.config.Config;
import breakit.mob.Mob;
import engine.graphics.Screen;
public class Ball extends Mob {
private int multiplier = 0;
public Ball(int x, int y) {
super(x, y);
setWidth(4);
setHeight(4);
setx(Config.WINDOW_WIDTH / 2);
sety(Config.WINDOW_HEIGHT / 2);
setxdir(Mob.LEFT);
setydir(Mob.UP);
setxspeed(2);
setyspeed(2);
}
@Override
public void update() {
setMobState(getMobState().update());
}
@Override
public void render(Screen screen) {
getMobState().render(screen);
}
public void addMultiplier(int n) {
multiplier += n;
}
public Integer getMultiplier() {
return multiplier;
}
public void setMultiplier(int n) {
multiplier = n;
}
}
I’m starting to see why composition is emphasized so heavily over inheritance, but mastering the art of decoupling is definitely a challenge. BTW I still have to read all the posts since I’ve been away, I just wanted to post an update of my progress. Sorry for the wall of code!
@cylab - Your post (reply # 29) showed me that you can use a class type as a key in a HashMap, so thank you for that. :point: I wanted to do that one point and didn’t know it was actually possible. Also, I like the implementation you suggest as it is very clean. I’ll probably try that at some point, but for now this implementation is working well.
I read in several spots on the thread that casting might smell of bad design, which I do in places, namely where a state is assigned to a Mob:
@Override
public MobState init(Mob m) {
brick = (Brick) m;
brick.setCol(0xffff00);
return this;
}
The problem is that I can’t assign the type and recognize the subclass methods without casting. It doesn’t generate a compile time error if you pass the wrong type as an argument, but I have tested it and it will throw a ClassCastException and print a stack trace. So I was wondering if this is considered bad design and if there is a better approach? Thank you all for contributing to the thread- your expert advice has been very enlightening!
In that example, it looks like your MobState super-class should be a generic type, with the init method using the class’ parameterized type for its parameter.
Something like:
abstract class MobState<T> {
public abstract MobState init(T m);
}
The return type could probably be parameterized too, though I’ve no idea what your MobState / Mob class heirarchy looks like.
hmm … removing the instanceof and having a blind cast is not improving things! Doing instanceof -> cast, using visitors, pattern matching or heterogenous containers are all about filtering a collection of entities at runtime for those that are a / have a particular capability. If you have a blind cast like that you either know at compile time the type returned, in which case that’s what the method should return, or you’re making an assumption that is overriding compile time checks and will eventually blow up on you at runtime.
@Abuse may be right that generics are the way to go here. Although your concrete subclasses probably don’t want to be generic or you’re likely to end up needing to know genetic types at runtime (a PITA!). So, something along the lines perhaps of BallStateNormal extends AbstractMobState
However, also don’t forget that overridden methods in Java can return a more specific subtype, so visitable.getMob() on BallStateNormal could return Ball already without generics.
And all this discussion, interesting discussion at that, could be solved with double dispatch.
The visitor pattern, indeed quite a few of the GoF patterns solve problems that often functional languages solve directly. Having said that double dispatch is not in many. Lisp and in particular CLOS in lisp and its derivatives.
In fact the Entity system i use does sort of do Generic methods like CLOS. It works pretty good.
OK, so I implemented generic typing on MobState and it worked swimmingly:
public interface MobState<T extends Mob> extends Visitable, Visitor {
public MobState<T> init(T t);
public MobState<T> update();
public void render(Screen screen);
public void processCollision(Mob m);
public T getMob();
public void setMob(T t);
public Visitor getVisitor();
}
So now Mob looks like this (Minus a bunch of fields and methods irrelevant to the thread, to save space):
public abstract class Mob {
private MobState<? extends Mob> ms = null;
private double x = 0;
private double y = 0;
public Mob(int x, int y) {
this.x = x;
this.y = y;
}
public final MobState<? extends Mob> getMobState() {
return ms;
}
public final void setMobState(MobState<? extends Mob> s) {
ms = s;
}
}
And an example of a concrete MobState object looks like this:
public class BrickStateSolid extends AbstractMobState<Brick> {
private Brick brick = null;
@Override
public MobState<Brick> init(Brick b) {
brick = b;
brick.setCol(0x808080);
return this;
}
@Override
public MobState<Brick> update() {
return this;
}
@Override
public void render(Screen screen) {
int c = brick.getCol();
int x = (int)brick.getx();
int y = (int)brick.gety();
int w = brick.getWidth();
int h = brick.getHeight();
for(int i = 0; i < brick.getHeight() / 2; i++) {
screen.drawRect(x++, y++, w, h, c);
w -= 2;
h -= 2;
c -= 0x202020;
}
}
@Override
public void processCollision(Mob m) {
accept(m.getMobState().getVisitor());
}
@Override
public Brick getMob() {
return brick;
}
@Override
public void setMob(Brick b) {
brick = b;
}
@Override
public void visit(BallStateNormal visitable) {
Ball ball = visitable.getMob();
brick.bounceBall(ball);
}
@Override
public void accept(Visitor visitor) {
}
}
Advantage: no type casting and no “instanceof”, meaning bad type-checks are caught at compile time.
So something like MobState mobState = new PlayerStateAlive().init(player); won’t compile, and once the type is set, there is no way to pass a bad object to a visitor object.
Disadvantage: kind of hard to read if you aren’t used to it. Specifically, I’m using <? extends Mob> in the Mob class, which is apparently the right one, because <? super Mob> generates this error at the return statement of MobState<? super Mob> getMobState(MobState<?super Mob>):
Type mismatch: cannot convert from MobState<capture#1-of ? extends Mob> to MobState<? super Mob>
However, <? super Mob> generates NO error in setMobState(MobState<? super Mob> ms)
<? extends Mob> was my first guess, but I tried <? super Mob> for the hell of it, and I really have no idea what the difference between the two is, other than I used <? super Object> in a data structures class that used it in a binary node interface: ``` public interface BinaryNodeInterface> ``` Whew! Java sure is verbose. Anyway, I was wondering if anybody had a good explanation of the difference between <? super >and <? extends>. I googled around and found some explanations, but it didn't really sink in for me. EDIT: as it turns out, the reason I was getting an error in MobState<? super Mob> getMobState() was because I hadn't set the type of the ms field to MobState<? super Mob> and it was still MobState<? extends Mob>. Once that was corrected, <? super Mob> still generated compiler errors wherever I actually tried to set the MobState: ``` MobState mobState = new BrickStateDestructible().init(brick); brick.setMobState(mobState); ``` The above statement would generate the error: ``` The method setMobState(MobState<? super Mob>) in the type Mob is not applicable for the arguments (MobState) ``` Simple enough: keep it <? extends Mob>. My best understanding is that with <? super Mob> brick.setMobState(mobState) would only accept of MobState and not MobState, MobState, etc. Since Mob is not specific enough, I don't think that would be very useful, as it would defeat the purpose of what I'm trying to accomplish in the first place.[quote]Whew! Java sure is verbose. Anyway, I was wondering if anybody had a good explanation of the difference between <? super >and <? extends>. I googled around and found some explanations, but it didn’t really sink in for me.
[/quote]
They are kind of like a greater than or less than, but referring to the extends chain. One gets the class and all super classes, the other gets the class and all sub classes.
Usually I end up trying every combination of super and extends until it compiles. The logic behind it is indeed logic, but so hard to grasp just by glancing at something that even now after, what, over 10 years of using generics, I still can’t get it right first time.
[quote]And all this discussion, interesting discussion at that, could be solved with double dispatch.
[/quote]
Heh And another language. But here we are.
Cas