Java "instanceof" to add over multiple lists

Question

I am currently trying to refactor my code after I read up that implementations are preferred over extensions. Currently, I am trying to create a function that adds an Object into a Scene. In order to better define what each object is, there are multiple Lists such as a list for updating, rendering, etc.

private List<Updatable> updatables;
private List<Removable> removables;
private List<Renderable> renderables;
private List<Collidable> collidables;

I want to make a function into my Scene class like so:

public void add(Object o) {
    if(o instanceof Updatable)
        updatables.add((Updatable) o);
    if(o instanceof Removable)
        removables.add((Removable) o);
    if(o instanceof Renderable)
        renderables.add((Renderable) o);
    if(o instanceof Collidable)
        collidables.add((Collidable) o);
}

likewise I would create a similar remove function. My question is if this is bad coding practice since I have heard of the pitfalls of instanceof, and secondly if it is, is there a way around this / restructuring my code to make adding instances simple? I prefer not to have to add an instance to every list and define which ones it belongs to every time.

Answer 1

I would do similar with Factory pattern using Map

static Map<String, List> maps = new HashMap<>();
static {
    maps.put(Updatable.class.getName(), new ArrayList<Updatable>());
    maps.put(Removable.class.getName(), new ArrayList<Removable>());
    maps.put(Renderable.class.getName(), new ArrayList<Renderable>());
    maps.put(Collidable.class.getName(), new ArrayList<Collidable>());
}
public static void add(Object obj){
    maps.get(obj.getClass().getName()).add(obj);
}

Answer 2

As you mentioned in the comments, your use case is somewhat special. Updatable, Removable, Renderable, and Collidable are all interfaces. You have classes which implement one or more of these interfaces. Ultimately, you want your add method to add an object to each list of which interface it implements. (Correct me if any of this is wrong.)

If overloading would be too verbose, you can accomplish this with reflection, like so:

private List<Updatable> updatables;
private List<Removable> removables;
private List<Renderable> renderables;
private List<Collidable> collidables;

@SuppressWarnings("unchecked")
public void add(Object o) {
    try {
        for (Class c : o.getClass().getInterfaces()) {
            // Changes "Updatable" to "updatables", "Removable" to "removables", etc.
            String listName = c.getSimpleName().toLowerCase() + "s";

            // Adds o to the list named by listName.
            ((List) getClass().getDeclaredField(listName).get(this)).add(o);
        }
    } catch (IllegalAccessException e) {
        // TODO Handle it
    } catch (NoSuchFieldException e) {
        // TODO Handle it
    }
}

But if you're going to use this approach, please note these caveats:

1. Reflection, by nature, is very prone to runtime errors.
2. You may run into issues if your hierarchy is more complex. For example, if you have Foo implements Updatable, Removable and also Bar extends Foo, then calling getClass().getInterfaces() on Bar will return an empty array. If this sounds like your use case, you may want to use ClassUtils.getAllInterfaces from Apache Commons Lang instead.

Answer 3

What I would expect is that you have a separate add method for each of these lists, like so:

public void addUpdatable(Updatable updatable) {
    updatables.add(updatable);
}

public void addRemovable(Removable removable) {
    removables.add(removable);
}

// and so on

The reason for this is that these are separate lists, and while the concern from the implementation perspective is to make the API elegant and slick, these efforts will affect clarity from the user's perspective negatively.

For instance, the misunderstanding (in EJP's answer comments) about what happens when an Updatable which is also a Removable is added shows this lack of clarity. Although you use similar terms for both your types and your state which makes it seem redundant, you should still make it clear which list (or part of the state) an item is added to.

In any case, it looks like you are struggling with a problem in its bigger scope. I think that it would be a good idea to invest more time toward understanding the bigger problem.

Answer 4

Just use overloads: add(Updateable u), add(Removable r), etc.

Answer 5

From @Jerry06, the best way to add an instance to multiple lists of different types for me was using a magic list getter.

private Map<Class, List> maps = new HashMap<>();

'maps' is used to find whichever list you are looking for.

private <T> List<T> get(Class<T> c) {
    return maps.get(c);
}

instead of using maps.get, I just use get(SomeClass.class). The function above automatically converts it to the list with correct element type. I then was able to do:

    get(Updatable.class).stream().filter(Updatable::isAwake).forEach(Updatable::update);

instead of:

    updatables.stream().filter(Updatable::isAwake).forEach(Updatable::update);

I mostly wanted a structure like this so all the lists could be wrapped up nicely, and secondly so I could just say maps.add(SomeNewClass.class, new ArrayList()), then call it whenever I wanted.