I do have an implementation issue that I condensed to the following code snippet, because the real code is much more complicated. The core problem is that I do have a container object that has a mixed list of persons, for example Managers and Engineers:
interface Person {
}
class Manager implements Person {
}
class Engineer implements Person {
}
In the most cases I have to access the list of persons without knowing the concrete subclass of a person. But in some methods I have to know the concrete type of the members, for example to count the members or to add the members. In both implementations I do have ugly code:
In implementation A it’s the code to separate the person-types for counting.
class EnterpriseA {
private final List<Person> members = new ArrayList<Person>();
public void addMember(Person person) {
this.members.add(person);
}
public void doSomethingWithManagers() {
// separate by instanceof
}
public void doSomethingWithEngineers() {
// separate by instanceof
}
}
In implementation B it’s the code to add a person to the right list of Manager or Engineer.
class EnterpriseB {
private final List<Manager> managers = new ArrayList<Manager>();
private final List<Engineer> engineers = new ArrayList<Engineer>();
public void addMember(Person person) {
if (person instanceof Manager) {
this.managers.add((Manager) person);
} else if (person instanceof Engineer) {
this.engineers.add((Engineer) person);
}
}
public void doSomethingWithManagers() {
// iterate over the managers.
}
public void doSomethingWithEngineers() {
// iterate over the engineers.
}
}
- How can I improve the code and avoid the ugly code in addMember() and countManagers() respectively? Smart suggestions are welcome.
- Which alternative would you prefer? Please give reasons, in which cases you would prefer which solution.
If these types are related – as in your example – I would say that solution A is superior.
It makes use of polymorphism – the fact that Manager and Employee are both Persons, so you could eg. create a list of all names or sum up their salaries without forcing your code to worry (or even know about) job title distinctions.
“Manual” type checks (instanceof) in solution B are typically a code smell and a pain when it comes to maintanability.
It’s also more bug-prone – even your sample code contained a bug already (confusing an engineer with a manager), which I already corrected.
If another type were to be added – say, Accountant – instead of only adding countAccountants, you have to insert another if check in addMember, and possibly various other methods.
Yes, there may be performance concerns regarding solution A. Having to filter the list and recount all Managers every time could be inefficient for big lists.
But there is a solution for that: you can keep count in the class and update counters in your addMember method, as well as any other method changing the contents of your collection (like removeMember), kind of caching the data.
You could even make it more generic and create a Map<Class<? extends Person>, Integer> typeToCount, where each key (Employee.class, Manager.class) would have a corresponding value representing how many objects of this particular type are stored. This would further decrease the effort of maintaining multiple types and protect you against bugs crawling in.
Like so:
class EnterpriseAPlus {
private final List<Person> members = new ArrayList<Person>();
private final Map<Class<? extends Person>, Integer> countByType =
new HashMap<Class<? extends Person>, Integer>();
public void addMember(Person person) {
members.add(person);
// (thread safety concerns disregarded for the sake of simplicity)
Class<? extends Person> personClass = person.getClass();
int currentCount;
if (countByType.containsKey(personClass)) {
currentCount = countByType.get(personClass);
} else {
currentCount = 0;
}
countByType.put(personClass, currentCount + 1);
}
public int countEngineers() {
return getCountOf(Engineer.class);
}
public int countManagers() {
return getCountOf(Manager.class);
}
protected final int getCountOf(Class<? extends Person> personClass) {
if (!countByType.containsKey(personClass)) {
return 0;
}
return countByType.get(personClass);
}
}
Need to handle Accountants now? Just add:
public int countAccountants() {
return getCountOf(Accountant.class);
}
No need to mess with addMember anymore. You could even add countAccountants by subclassing EnterpriseAPlus – remember the open/closed principle…
3