CODEWITHSUNDEEP
javaoop
Chris
Chris

Reputation: 9529

Refactoring huge if ( ... instanceof ...)

I'm currently trying to refactor a part of a project that looks like this:

Many classes 

 B extends A; C extends A; D extends C; E extends B; F extends A; ...

And somewhere in the code:

if (x instanceof B){
 B n = (B) x;
 ...
}else if (x instanceof C){
 C n = (C) x;
 ...
}else if (x instanceof D){
 D n = (D) x;
 ...
}else if (x instanceof E){
 E n = (E) x;
 ...
}else if (x instanceof G){
 G n = (G) x;
 ...
}...

Above if-construct currently sits in a function with a CC of 19. Now my question is: Can I split this if-construct up in mutliple functions and let Java's OO do the magic? Or are there any catches I have to look out for?

My idea:

private void oopMagic(C obj){ ... Do the stuff from the if(x instanceof C) here}
private void oopMagic(D obj){ ... Do the stuff from the if(x instanceof D) here}
private void oopMagic(E obj){ ... Do the stuff from the if(x instanceof E) here}
....

and instead of the huge if:

oopMagic(x);

Edit: I cannot change any of the classes (A,B,C,...). Inside the if statements some getters are used to read (never write) data from each object.

Upvotes: 4

Views: 3507

Answers (5)

Andreas Dolk
Andreas Dolk

Reputation: 114817

If you can't change the the classes, you may still be able to wrap them and introduce a factory to create a correct wrapper for your type:

public interface Wrapper {
  public void magicMethod();  // you know what I mean
}

public AWrapper implements Wrapper {
  private A a;
  public AWrapper(A a){this.a=a;};

  @Override
  public void magicMethod() {
    // do what has to be done with a
  }
}

public Factory {
  public static createWrapper(Object wrappable) {
    if (wrappable instanceof A)
        return new AWrapper((A) wrappable);
    // ...
  }
}

// ...
Object x = getXFromSomewhere();
Wrapper wrapper = Factory.getWrapper(x);
wrapper.magicMethod();
// ...

Sure, this will not eliminate the sequence of instanceof statements but it will move it to a factory and that's at least a better place. Factory methods almost always contain some conditional checks which are needed to create the correct object.

Upvotes: 2

nanda
nanda

Reputation: 24808

I'm afraid (as also suggested by one commenter) you just move the problem somewhere else, e.g. the if-else will be done in other place. There is no problem with your approach but to make it better you have to do some more work.

What I suggest is to create a Map that will process x according to its type. Processor itself is a generic interface, something like:

interface Processor<T extends A> {
     void oopMagic(T obj);
}

Then create implementation of the processor for each class, something like:

class ProcessorB<B> {
     void oopMagic(B obj) { ... }
}

Put all implementation class in the map and then do:

map.get(x.getClass()).oopMagic(x);

Upvotes: 1

Little Bobby Tables
Little Bobby Tables

Reputation: 5351

That won't fly. instanceof detects the runtime type of a variable. Polymorphism (select method by signature) depends on the compile-time type of a variable. That is, you'll always get oopMagic(A obj).

As Roger suggested, take a look at the visitor pattern, a.k.a double-dispatch.

Upvotes: 5

Roger Lipscombe
Roger Lipscombe

Reputation: 91935

Depending on what's in those ...s, you could get away with making a virtual (maybe abstract) method doMagic in the base class and then just:

x.doMagic();

If the ... parts are sufficiently different, then (apart from having other problems with the design), you could look at implementing double-dispatch using the visitor pattern.

Upvotes: 0

Brad Mace
Brad Mace

Reputation: 27916

It's hard to tell what would work best since you haven't given any indication what the code is doing, but another option is to add an oopMagic() method to A and override as necessary.

Upvotes: 2

Related Questions