What is wrong with this design? Overriding or overloading of java.util.HashMap

Question

This question was asked to me in MS interview. I wanna know the exact design issue in this piece of code. Code was already given, needed to find the design issue.

I have class MyHashMap which extends java HashMap class. In MyHashMap class I have to keep some information of employees. Key in this map will be firstName+lastName+Address .

public MyHashMap extends HashMap<Object, Object> {
  //some member variables 
  //
  public void put(String firstName, String lastName, String Address, Object obj) {
       String key =   firstName + lastName+ Address;
       put(key, obj);
  }

  public Object get(String firstName, String lastName, String Address) {
       String key =   firstName + lastName+ Address;
       return get(key);
  }

  public void remove(Strig key) {
        put(key, ""); 
  }

  //some more methods 
}

What is wrong with this design? Should I subclass HashMap or should I declare HashMap as member variable of this class? Or should I have implemented hashCode/equals methods?

Answer 1

There are quite a few problems, but the biggest problem I can see it that you're using a concatenated String as a key. The following two calls are different, but equivalent:

final MyHashMap map = new MyHashMap();

map.put("foo", "", "baz", new Object());
map.put("", "foo", "baz", new Object()); // Overwrites the previous call

There's also an issue that you're declaring that the key type as an Object, but always using String and are therefore not taking advantage of the type safety that comes with generics. For example, if you wanted to loop through the keySet of your Map, you'd have to cast each Set entry to a String, but you couldn't be sure that someone didn't abuse you Map by using an Integer key, for example.

Personally, I would favour composition over inheritance unless you have a good reason not to. In your case, MyHashMap is overloading the standard Map methods of put, get and remove, but not overriding any of them. You should inherit from a class in order to change its behaviour, but your implementation does not do this, so composition is a clear choice.

To act as an example, overloading rather than overriding means that if you make the following declaration:

Map<Object, Object> map = new MyHashMap();

none of your declared methods will be available. As recommended by some of the other answers, it would be far better to use an object composed of firstName, lastName and address to act as your map key, but you must remember to implement equals and hashCode, otherwise your values will not be retrievable from the HashMap.

Answer 2

What's wrong with that design is primarily that is claims to be a HashMap<Oject, Object>, but isn't really. The overloaded methods "replace" the Map methods, but those are still accessible - now you're supposed to use the class in a way that is incompatible with the Map interface and ignore the (still technically possible) compatible way to use it.

The best way to do this would be to make an EmployeeData class with name and address fields and hashCode() and equals() methods based on those (or, better yet, a unique ID field). Then you don't need a non-standard Map subclass - you can simply use a HashMap<EmployeeData, Object>. Actually, the value type should be more specific than Object as well.

Answer 3

I would go for declaring HashMap as member variable of your class.

I don't remember if a great explanation is given in Clean Code or Effective Java, but basically, it's simplier to do, requires less work if the API changes.

generally, extending such a class means you want to change its behavior.