Что не так с этим дизайном? Переопределение или перегрузка java.util.HashMap

Question

Этот вопрос был задан мне в интервью MS. Я хочу знать точную проблему дизайна в этом куске кода. Код уже предоставлен, необходимо найти проблему с дизайном.

У меня есть класс MyHashMap, который расширяет класс Java HashMap. В классе MyHashMap я должен хранить некоторую информацию о сотрудниках. Введите эту карту: 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 
}

Что не так с этим дизайном? Должен ли я подкласс HashMap или я должен объявить HashMap как переменная-член этого класса? Или я должен был реализовать методы hashCode / equals?

Answer 1

Есть довольно много проблем, но самая большая проблема, которую я вижу, это то, что вы используете сцепленный String в качестве ключа. Следующие два вызова различны, но эквивалентны:

final MyHashMap map = new MyHashMap();

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

Существует также проблема, связанная с тем, что вы объявляете тип ключа как Object, но всегда используете String и, следовательно, не пользуетесь безопасностью типов, которая поставляется с обобщениями. Например, если вы хотите перебрать keySet вашего Map, вам придется привести каждую Set запись к String, но вы не можете быть уверены, что кто-то не оскорбил вас. Map с помощью клавиши Integer, например.

Лично я бы предпочел бы композицию, а не наследование , если у вас нет веских причин не делать этого. В вашем случае MyHashMap - это перегрузка стандартные Map методы put, get и remove, но не перекрывают ни один из них. Вы должны наследовать от класса, чтобы изменить его поведение, но ваша реализация этого не делает, поэтому составление - это очевидный выбор.

Например, перегрузка, а не переопределение означает, что если вы сделаете следующее объявление:

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

нет из ваших заявленных методов будут доступны. В соответствии с рекомендациями некоторых других ответов, было бы гораздо лучше использовать объект, состоящий из firstName, lastName и address, в качестве ключа карты, но вы должны помнить о реализации equals и hashCode, иначе ваши значения будут не может быть извлечено из HashMap.

Answer 2

Что плохо с этим дизайном, так это то, что он претендует на HashMap<Oject, Object>, но на самом деле это не так. Перегруженные методы «заменяют» методы Map, но они все еще доступны - теперь вы должны использовать класс способом, несовместимым с интерфейсом Map, и игнорировать (все еще технически возможный) совместимый способ используйте это.

Лучший способ сделать это - создать класс EmployeeData с полями имени и адреса и методами hashCode() и equals() на их основе (или, что еще лучше, с уникальным полем идентификатора). Тогда вам не нужен нестандартный подкласс Map - вы можете просто использовать HashMap<EmployeeData, Object>. На самом деле, тип значения должен быть более конкретным, чем Object.

Answer 3

Я хотел бы объявить HashMap переменной-членом вашего класса.

Я не помню, чтобы было дано хорошее объяснение в Чистом коде или в Эффективной Java, но, в принципе, это проще, требует меньше работы при изменении API.

Как правило, расширение такого класса означает, что вы хотите изменить его поведение.

Answer 4

Карта использует внутренний ключ на основе имени, фамилии и адреса. Таким образом, метод remove должен (1) быть реализован как remove(String firstName, String lastName, String Address) и (2) он не должен изменять поведение исходного метода удаления (который действительно удалил запись), просто изменяя значение. Лучшая реализация удаления будет:

public void remove(String firstName, String lastName, String address) {
   String key =   firstName + lastName + address;
   return remove(key);
}

Answer 5

Мой первый дубль будет таким:

public MyHashMap extends HashMap<Oject, Object>

неправильно. Нет безопасности типа.

Если для карты требуется , то хотя бы сделайте ее

public MyHashMap extends HashMap<NameAddress, Employee>

и настройте пут и получите то же самое. NameAddress использовать имя, фамилию и адрес в равных и hashCode.

Лично я чувствую, что интерфейс Set будет лучше соответствовать, возможно, в качестве члена HashMap. Тогда вы можете определить интерфейс следующим образом:

public EmployeeSet implements Set<Employee> {
  private static class NameAddress {
    public boolean equals();
    public int hashCode();
  } 

  private HashMap employees = new HashMap<NameAddress, Employee>();   

  public add(Employee emp) {
    NameAddress nad = new NameAddress(emp);
    empoyees.add(nad, emp);
  }

  public remove(Employee emp) {
  }
}

и извлеките информацию об имени и адресе в реализации для создания ключа.

РЕДАКТИРОВАТЬ Дэвид указал, что NameAddress не должен быть сопоставимым, и я удалил эту часть интерфейса. Добавлен хеш-код для корректности.

Answer 6

Два других "отвратительных преступления" против надлежащей практики заключаются в том, что метод remove:

• перегружает Map.remove(<K>) вместо того, чтобы переопределять его (так что у вас есть утечка абстракции), и

• реализует поведение, несовместимое с поведением метода, который он переопределяет!

Установка значения, связанного с ключом, в пустую строку НЕ ЖЕ, как удаление записи для ключа. (Даже установка на null немного отличается ...)

Это не является нарушением принципа замещаемости Лискова, но это может сбить с толку тех, кто пытается использовать этот тип. (В зависимости от того, вызвали ли вы метод с помощью типа Map или типа MyHashMap, вы можете использовать разные методы с различной семантикой ....)

Answer 7

Я думаю, это во многом зависит от контекста и того, что именно они спросили. Например, это высококонкурентный контекст, вы должны были использовать вместо него Hashtable. Также вы должны указать типы (String, Object) вместо (Object, Object), чтобы получить поддержку компилятора для ключей.

Я думаю, что лучше было бы реализовать интерфейс Map и сохранить HashMap / HashTable в качестве внутреннего параметра объекта.