0

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.
    }
}
  1. How can I improve the code and avoid the ugly code in addMember() and countManagers() respectively? Smart suggestions are welcome.
  2. Which alternative would you prefer? Please give reasons, in which cases you would prefer which solution.
| improve this question | | | | |
3

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...

| improve this answer | | | | |
  • I updated my code example and changed the count-methods to do-mehtods, because counting is a very easy example. The do methods in A also need an instanceof-switch to separate the instances. – shylynx Oct 9 '14 at 12:52
  • This changes the landscape a bit :) It probably shifts the situation in favor of solution B. But your example is too contrived for me to say. You are nowhere actually making use of the fact that all the objects are Persons (addMember could be addMember(Object object) just the same), which begs the question of why you included Person in your sample code at all? If having a common base class is irrelevant (and we don't take any advantage of it), then maintaining separate collections in one umbrella class could be the way to go. – Konrad Morawski Oct 9 '14 at 13:03
  • 1
    Note that keeping a List<Person> allows you to use a Visitor pattern, in which a visitor object is given access to consecutive Person entities and is able to decide by itself whether a certain action should be applied to them or not. The criteria could be more sophisticated than just a job title. This allows for better modularity of code. – Konrad Morawski Oct 9 '14 at 13:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.