0

I have these interfaces:

public interface IShipOwner {}

public interface ICitizen {}

public interface IPlayer extends ICitizen, IShipOwner {}

public interface IAIPlayer exends IPlayer {}

Further I have these two methods in the same class:

public boolean isHumanPlayer(ICitizen citizen) {
    if (citizen instanceof IPlayer) {
        if (citizen instanceof IAIPlayer) {
            return false;
        } else {
            return true;
        }
    }
    return false;
}

public boolean isHumanPlayer(IShipOwner shipOwner) {
    if (shipOwner instanceof IPlayer) {
        if (shipOwner instanceof IAIPlayer) {
            return false;
        } else {
            return true;
        }
    }
    return false;
}

When calling isHumanPlayer with an object of type IPlayer I have to cast it either to type ICitizen or IShipOwner to make it clear which method should be called.

What is the best way from a client perspective for calling these methods? If possible I do want to avoid the need to cast the parameter while at the same time retaining the method overloading.

  1. Make citizen implement IShipOwner even if not every IShipOwner is a ICitizen and vice versa.
  2. Use different method names.
  3. Something else I am not thinking of.
hotzst
  • 7,238
  • 9
  • 41
  • 64
  • Maybe an abstract `Player` class with a `boolean isHuman()` method? – OneCricketeer Dec 09 '15 at 20:18
  • Agree. Each class which implements/extends interface/abstract class, will define `isHumanPlayer` method if necessary (if for IPlayer you define default value) – Valijon Dec 09 '15 at 20:21
  • 1
    It would be nice to know more about relations between classes. – leonz Dec 09 '15 at 20:23
  • The relations between the implementing classes are like the ones defined on the interface level. However there are many more instances of `ICitizen` than `IPlayer`. As stated there is no direct correlation between `ICitizen` and `IShipOwner` even if they share some of the same methods (like `getName()`) – hotzst Dec 09 '15 at 20:27
  • 1
    Unrelated to this question - http://stackoverflow.com/a/2814831/24396 – Steve Kuo Dec 09 '15 at 20:27
  • @Nathan_Hughes: If I call the method with an instance of `IPlayer` will not compile as `IPlayer` is both of type `ICtizen` and `IShipOwner`. – hotzst Dec 10 '15 at 07:27

2 Answers2

2

Testing an object's capabilities is a code smell. It means you're not utilizing the OO features that are built into the language. You should probably get rid of your isHumanPlayer methods entirely. Then wherever you were testing isHumanPlayer and doing different things depending on the result, just call one method on IPlayer that does different things depending on the implementation.

My suggestion:

  • interface Player - Common behavior goes here, including a method for whatever it was you were going to do based on the value of isHumanPlayer.
  • interface HumanPlayer extends Player - Adds methods that are only relevant to human players.
  • interface AiPlayer extends Player - Adds methods that are only relevant to AI players.

The methods added by HumanPlayer and AiPlayer should be called only by parts of your program that deal with players specifically as human or AI players. You should rarely if ever find it necessary to cast. If you do, the methods you're calling on the casted object might need to be moved up the hierarchy.

Based on the existing hierarchy you described, it sounds like human and AI players are both ShipOwners. But exactly where the ShipOwner interface fits in depends on whether there is any other kind of Player that is not a ShipOwner, or any kind of ShipOwner that is not a Player.

Kevin Krumwiede
  • 9,868
  • 4
  • 34
  • 82
  • But would that not mean a difference inheritance as `IAIPlayer` is a sub interface of `IPlayer`? Maybe an additional interface between `ICitizen` and `IPlayer` from which then `IPlayer` and `IAIPlayer` extend thereby uncoupling the `IAIPlayer` form `IPlayer`. – hotzst Dec 09 '15 at 21:46
  • @hotzst I'm confused. Which interface represents a human player, and which is the common super-interface of human and AI players? – Kevin Krumwiede Dec 09 '15 at 21:53
  • The hierarchy as it is goes IAIPlayer extends IPlayer extends ICitizen. IPlayer also implements IShipOwner. However I could imagine that another inheritance hierarchy might be better suited. – hotzst Dec 09 '15 at 21:57
  • I believe changing the inheritance hierarchy is the proper approach. – hotzst Dec 12 '15 at 20:10
1

I second Kevin Krumwiede's opinion.

But if you really must keep isHumanPlayer(), then:

Make citizen implement IShipOwner even if not every IShipOwner is a ICitizen and vice versa.

No. Never. "If there you go, only pain will you find."

Use different method names.

That would work, but it would be tantamount to accepting defeat, right?

Something else I am not thinking of.

Which brings us to:

  • Add isHumanPlayer( IPlayer player )

  • Come up with a new interface which is extended by all other interfaces, perhaps IPerson.

  • Make isHumanPlayer() part of each interface. So, human players would simply return true; everyone else would simply return false.

Also please note that this whole mess:

public boolean isHumanPlayer(ICitizen citizen) {
    if (citizen instanceof IPlayer) {
        if (citizen instanceof IAIPlayer) {
            return false;
        } else {
            return true;
        }
    }
    return false;
}

can be restated as follows:

public boolean isHumanPlayer(ICitizen citizen) 
{
    return citizen instanceof IPlayer && !(citizen instanceof IAIPlayer);
}

(Which may and may not be what your original intention was.)

Mike Nakis
  • 56,297
  • 11
  • 110
  • 142