Why is "instanceof" not working?

Question

I'm using the Java instanceof but it doesn't seem to be working.

I have three java classes that extend a Hero class.
The Hero.java class:

public abstract class Hero {

    protected int health;

    public Hero() { 
    }
}

The other three classes:

public class Archer extends Hero {
    public Archer() {
    }
}

public class Mage extends Hero {
    public Mage() {
    }
}

public class Warrior extends Hero {
    public Warrior() {
    }
}

I have this main class WelcomeScreen.java

public class WelcomeScreen {

    private Archer archer;
    private Mage mage;
    private Warrior warrior;
    private Hero hero;

public WelcomeScreen() {

        // choose a hero (archer/mage/warrior)
        hero = archer;
        new Game(hero);
    }

    public static void main(String args[]) {
        new WelcomeScreen();
    }

}

that instantiates the Game.java class

public class Game {

    public Game(Hero chosenHero) {

        if (chosenHero instanceof Mage) {
            System.out.println("you selected mage");
        } else if (chosenHero instanceof Archer) {
            System.out.println("you selected archer");
        } else if (chosenHero instanceof Warrior) {
            System.out.println("you selected warrior");
        } else {
            System.out.println("you selected NOTHING");
        }
    }

}

In Game.java, the code is meant to check whether chosenHero is an object of Archer.java, Warrior.java, or Mage.java, but I result with "you selected NOTHING". Why does instanceof fail to check if I already assigned it to Archer.java in the WelcomeScreen?

archer variable is null. -- where do you call `new Archer()` anywhere? — Hovercraft Full Of Eels, Jun 10 '16 at 02:10
You will want to learn to use a debugger, and then practice using it. Also heavy use of `instanceof` is structure that gives off a code smell and should be avoided. — Hovercraft Full Of Eels, Jun 10 '16 at 02:11
@HovercraftFullOfEels umm... okay... What does it smell like? — RobotKarel314, Jun 10 '16 at 02:25
@RobotKarel314: I don't know as I've never whiffed it close up, but I know that it's [not a good smell](https://www.google.com/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=java%20instanceof%20code%20smell). — Hovercraft Full Of Eels, Jun 10 '16 at 02:33

score 3 · Accepted Answer · edited May 23 '17 at 11:58

3

Because your constants are null. When you say,

private Archer archer;

it is equivalent to

private Archer archer = null;

Additionally, you have created three fields per instance. I think you wanted to do something like

private static final Hero archer = new Archer();
private static final Hero mage = new Mage();
private static final Hero warrior = new Warrior();

See also What does it mean to “program to an interface”?

edited May 23 '17 at 11:58

Community

1
1

answered Jun 10 '16 at 02:11

Elliott Frisch

198,278
20
158
249

Why do you think they should be static? – shmosel Jun 10 '16 at 02:13
@shmosel because they appear to be global character types for some kind of RPG (Having four `Hero` fields for a single player seems a bit odd). Alternatively, they should be eliminated entirely and the op could just have one instance `Hero hero` field (`Hero hero = new Archer();`) – Elliott Frisch Jun 10 '16 at 02:15
I suspect `hero` is supposed to be an instance variable of `WelcomeScreen` and the previous types were because OP didn't understand how to assign new instances. – shmosel Jun 10 '16 at 02:17
I see. I actually thought there was a way to only instantiate the class the user chooses. I never wanted to create all of them (and use the memory). – Dragneel Jun 10 '16 at 02:18
@LawrenceLelo Sure, but the user has to specify them (you have it hard coded to "archer"). – Elliott Frisch Jun 10 '16 at 02:19

score 2 · Answer 2 · answered Jun 10 '16 at 02:46

2

Alternative solution: get rid of instanceof as it suggests a brittle rigid design, one that's easily broken. Instead try to use other more OOP-compliant solutions such as inheritance, or if complex, a Visitor Design Pattern.

For example, a simple inheritance structure could look something like:

public class WelcomeScreen {
    public WelcomeScreen() {

        // choose a hero (archer/mage/warrior)
        Hero hero = new Archer();
        new Game(hero);
    }

    public static void main(String args[]) {
        new WelcomeScreen();
    }
}

abstract class Hero {
    protected int health;
    // other shared fields such as String name,...

    public Hero() {
    }

    public abstract String getType();

    public int getHealth() {
        return health;
    }

}

class Archer extends Hero {
    public static final String TYPE = "Archer";

    public Archer() {
    }

    @Override
    public String getType() {
        return TYPE;
    }
}

class Mage extends Hero {
    public static final String TYPE = "Mage";

    public Mage() {
    }

    @Override
    public String getType() {
        return TYPE;
    }

}

class Warrior extends Hero {
    public static final String TYPE = "Warrier";

    public Warrior() {
    }

    @Override
    public String getType() {
        return TYPE;
    }

}

class Game {

    private Hero hero;

    public Game(Hero chosenHero) {
        this.hero = chosenHero;
        System.out.println("You selected a hero of type " + hero.getType());
    }

}

answered Jun 10 '16 at 02:46

Hovercraft Full Of Eels

283,665
25
256
373

Alright, let me ask you something. What if the Archer's constructor needs a Game object as parameter? Like so: **public Archer(Game game) {};** Because in that case this will not work since the Archer object is being created before Game.java. Any thoughts? – Dragneel Jun 10 '16 at 03:21
@LawrenceLelo: your question above is a red-herring question since it has nothing to do with the instanceof issue. The simple solution would be to re-write Archer's constructor to allow Game, but again, this has nothing to do with your main problem. – Hovercraft Full Of Eels Jun 10 '16 at 03:23
I'm fully aware that it has nothing to do with my original problem. But I mention it because I thought of your solution and ran into this exact issue. Nonetheless, your solution is very helpful. – Dragneel Jun 10 '16 at 03:25
@LawrenceLelo: I don't see where the problem would be. You'd simply give whatever class that needs to pass Game into its constructor a constructor that allows it. My solution would not prevent this, nor would the other solution. – Hovercraft Full Of Eels Jun 10 '16 at 03:27
The problem lies under WelcomeScreen's constructor. There can only be 1 object of Game.java, and 1 object of Archer.java. So how can Archer pass Game as a parameter if Game needs Archer as a parameter? – Dragneel Jun 10 '16 at 03:33
1

@LawrenceLelo: No requirements need be written in stone. You could simply use a setter method for one or both. Or if you really want to be fancy and decouple everything, use a dependency injection framework such as Spring or Guice. – Hovercraft Full Of Eels Jun 10 '16 at 03:36

Why is "instanceof" not working?

2 Answers2