Java - My Code is clearly going against common OOD paradigms, but not sure how to improve it [closed]

Question

EDIT: I really appreciate everyone's input. I gained something from all the responses and learned a good deal about OOD.

I am making a simple virtual tabletop war game. To represent units on the battlefield I have the following simple class hierarchy: An abstract class Unit, and two derived classes, Troop and Vehicle.

I have another class that has a hashtable for all the units in the game. The hashtable values are of Unit type, so I can reference them in O(1) time.

For the most part, this is fine, but sometimes the caller NEEDS to know if something is a troop or a vehicle to call specific methods from those derived classes. To accommodate for this, I've created two get methods that will enforce the types:

  public Troop getTroop(String uniqueID) {
    Unit potentialTroop = get(uniqueID);
    if(potentialTroop instanceof Vehicle) {
      throw new InternalError();
    }
    return (Troop) potentialTroop;
  }

  public Vehicle getVehicle(String uniqueID) {
    Unit potentialVehicle = get(uniqueID);
    if(potentialVehicle instanceof Troop) {
      throw new InternalError();
    }
    return (Vehicle) potentialVehicle;
  }

(Note the class for which this belongs merely extends Hashtable, so the get method being used here is the Java's hashtable's get method.)

So I think this is poor OOD design because if I ever further extend unit I'm going to have to add more checks and more #get methods to this hashtable.

Am I correct in saying this? Does anyone have alternative OOD suggestions if this is the case?

Answer 1

Here's a simple way to do this, not necessarily the best, but it meets these requirements:

1. Be able to dynamically obtain a specialized type from your Unit collection
2. Be able to add additional Unit types later on, without having to add a bunch of handler methods.

The solution uses a 'template' class to perform matching:

@SuppressWarnings("unchecked")
public <T extends Unit> T getSpecializedUnitType(Class<T> unitTypeClass, String uniqueID) {
    Unit potentialTroop = units.get(uniqueID);
    if(potentialTroop == null) return null;

    return potentialTroop.getClass().equals(unitTypeClass) ?
        (T) potentialTroop : null;
}

I made the assumption that you are going to correct your code, to not extend from Map, but rather to encapsulate it.

Answer 2

I would not extend HashTable (or any other class) in this case. Your class could use a HashTable internally, but by extending it you expose a lot of public API. The less you expose the better. You should generally favour composition over inheritance.

You would then have more flexibility in how you store the objects. In your case, you could have 3 maps internally; one containing all units, one just for Troops and one just for Vehicles. A given Unit would be stored in two maps, so you'd have to synchronize the adding and removing of Units to ensure integrity between the various maps.

Answer 3

Why not implement only one get method that returns Unit? I believe that if the two classes have very specific, different methods, then they don't have a lot in common, do they? The fact that you want them to be subclasses of Unit makes me think that their similarity is beyond the fact "they are objects with a position on the map". In this case, the "wrong" part would be giving the methods different names and calling them separately. A way to make things better would be

• to return only Unit from the hashtable
• as already suggested, to make comprehensive methods when possible, like behave(), move(), that could be common to both classes and could therefore be called without casting (even though they would do different things). Use the hierarchy you created! These methods would eventually call the small, modular methods you created before (that would now become private).
• you don't always need to know the type of Unit, do you? Only when necessary (not solvable with the techniques mentioned above), delegate such work to what you defined as "the caller", which I don't believe is a single, fixed entity. In other words, perform a check on type only when you have to decide whether your Unit shoots with the rifle or not. When you want to perform common tasks, there is no need to anticipate such distinction.