Alternative to instanceof approach in this case

Question

Hello I'm wondering what would be some more elegant alternatives to something like this:

class Base...

class A extends Base...

class B extends Base...

//iterator of colection containing mixed As and Bs i want to remowe Bs and do omething with As
while(iterator.hasNext()) {
    Base next = iterator.next();
    if(next instanceof A) // do something
    if(next instanceof B)
        iterator.remove();
}

Sow what are the alternatives...

Thank you for advices.

edit: Base class may have many subclasses not just two and their numbers may grow in time

score 1 · Answer 1 · answered Feb 21 '11 at 10:37

You can create methods in Base and override them in A and B.

For example:

class Base{
    public boolean shouldRemove(){
        return false;
    }
    public void doSomething(){
    }
}

class A extends Base{
    @Override
    public void doSomething() {            
    }
}

class B extends Base{
    @Override
    public boolean shouldRemove() {
        return true;
    }
}

and then you don't need know what class the object is an instance of:

    while(iterator.hasNext()) {
        Base next = iterator.next();
        if(next.shouldRemove()){
            iterator.remove();
        }
        else{
            next.doSomething();
        }
    }

Mario Duarte · Answer 2 · 2011-02-25T17:17:35.660

Do you really need to remove them from the list? Why don't you just have the method to do something in the Base class (doing nothing) and then just override it do to what you want on class A.

class Base{
    public void doSomething(){
    }
}


class A extends Base{
    @Override
    public void doSomething(){
        // do something
    }
}

Then you could just iterate over the list and calling the method doSomething on all objects.

for(Base base : list) {
    base.doSomething();
}

This way only the classes that have overridden the doSomething() method will actually do something. All the other classes will just execute the dummy implementation in the Base class.

If Base was an abstract class you could declare the doSomething() as abstract and have the extending classes implement it. With this approach all classes would have to implement the method and classes for which you don't want any computation to be performed you would just provide a dummy implementation of that method. Alternatively you could even create an interface with the doSomething() method and have (which could even be a better decision) and have the Base class implement it, given that only the extending classes would actually implement the method.

Sergey Vedernikov · Answer 3 · 2011-02-21T10:41:51.497

i think is very short and clear solution and has no alternatives (without code growing), just add else if instead of if in second case

Also you can split code on function calls, and if statement will not be huge

Another solution is to create Map of delegates that will be called. Like this: interface ISimpleDelegate{ void doSomeLogic(Base b) } `Map delegates = new HashMap();

After this add your logic as anonymous classes that realizes ISimpleDelegate. delegates.put(A.class, new ISimpleDelegate() { //write your logic here });

I hope that the idea is clear

And in your loop you just call delegates:

while(iterator.hasNext()) {
    Base next = iterator.next();
    delegates.get(next.getClass()).doSomeLogic(next);
}

score 0 · Answer 4 · answered Feb 21 '11 at 10:36

instanceof is a good way to filter objects by type - and that's what you want to do. You have a mixed collection and so you need some kind of filter, either filter the input (store nothing but As) or filter the output (process nothing but As).

If you just don't like "instanceof", you could use an enum to specify the type and add a final method to get the type at Base:

enum Type { ATYPE, BTYPE };

public Base {

   final private Type type;
   public Base(Type type) { this.type = type; }
   public Type getType() { return type; }
   // ...
}

public A {
   public A() { super(Type.ATYPE); }
}

while(iterator.hasNext()) {
    Base next = iterator.next();
    switch (next.getType) {
      case ATYPE: // do something and break
      case BTYPE: iterator.remove(next); break;
    }
}

Philipp Wendler · Answer 5 · 2011-02-21T12:03:43.557

In general, a nice solution to avoid instanceof is to use the so-called visitor pattern.

For this pattern, you need an additional interface (the Visitor), an implementation of it that contains the code you want to execute and an additional method in all classes of your hierarchy, so this might be overkill in small cases (but it is very handy if there is not only A and B, but more types).

In your case it would look like this:

interface Visitor {
  void visit(A a);
  void visit(B b);
}

class Base {
  abstract accept(Visitor v);
}

class A extends Base {
  accept(Visitor v) {
    v.visit(this);
  }
}

class B extends Base {
  accept(Visitor v) {
    v.visit(this);
  }
}

class MyVisitor implements Visitor {
  visit(A a) {
    doSomethingWithA(a);
  }

  visit(B b) {
    doSomethingWithB(b);
  }
}

It is used like this:

MyVisitor v = new MyVisitor();
while(iterator.hasNext()) {
    Base next = iterator.next();
    next.accept(v);
}

An advantage is that you have to write most of the code only once. If you want to do other things with A and B in another place of your program, just write another implementation of Visitor. You don't need to modify Base, A and B as you would if you'd add doSomething() to these classes.

Edit: If the number of sub-classes increases, you need to change all your existing implementations of Visitor. However, at least the compiler tells you about that. With instanceof you might end up forgetting a place where you need to add a handling clause. This can at most be detected at runtime, whereas the visitor pattern gives you compile-time safety.

Alternative to instanceof approach in this case

5 Answers5

Linked