Need help make these classes use Visitor Pattern and generics

Question

Hi.

I need help to generify and implement the visitor pattern. We are using tons of instanceof and it is a pain. I am sure it can be modified, but I am not sure how to do it.

Basically we have an interface ProcessData

public interface ProcessData {
  public setDelegate(Object delegate);
  public Object getDelegate();
  //I am sure these delegate methods can use generics somehow
}

Now we have a class ProcessDataGeneric that implements ProcessData

public class ProcessDataGeneric implements ProcessData {
  private Object delegate;

  public ProcessDataGeneric(Object delegate) {
    this.delegate = delegate;
  }
}

Now a new interface that retrieves the ProcessData

interface ProcessDataWrapper {
  public ProcessData unwrap();
}

Now a common abstract class that implements the wrapper so ProcessData can be retrieved

@XmlSeeAlso( { ProcessDataMotorferdsel.class,ProcessDataTilskudd.class })
public abstract class ProcessDataCommon implements ProcessDataWrapper {
  protected ProcessData unwrapped;

  public ProcessData unwrap() {
    return unwrapped;
  }
}

Now the implementation

public class ProcessDataMotorferdsel extends ProcessDataCommon {

  public ProcessDataMotorferdsel() {
    unwrapped = new ProcessDataGeneric(this);
  }
}

similarly

public class ProcessDataTilskudd extends ProcessDataCommon {

  public ProcessDataTilskudd() {
    unwrapped = new ProcessDataGeneric(this);
  }
}

Now when I use these classes, I always need to do instanceof

ProcessDataCommon pdc = null;
if(processData.getDelegate() instanceof ProcessDataMotorferdsel) {
   pdc = (ProcessDataMotorferdsel) processData.getDelegate();
} else if(processData.getDelegate() instanceof ProcessDataTilskudd) {
   pdc = (ProcessDataTilskudd) processData.getDelegate();
}

I know there is a better way to do this, but I have no idea how I can utilize Generics and the Visitor Pattern. Any help is GREATLY appreciated.

UPDATE

I want to add that these classes are just snippets of a much larger implementation. The ProcessData and ProcessDataGeneric is something that is outside of the delegates (ProcessDataMotorferdsel and so on). The delegates all extends ProcessDataCommon.

I can agree that a refactoring is probably best to do, but this is production code that is 2 years old, and it is costly to refactor (time,testing, etc). However, I am willing to do it.

UPDATE #2

I have tried to start the Generic process, however I am getting compile error. This is how it looks now.

public interface ProcessData<T extends ProcessDataCommon> {
  T getDelegate();
  setDelegate(T delegate);
}

public class ProcessDataGeneric<T extends ProcessDataCommon> implements ProcessData<T> {
  private T delegate;
  //Getter & setter
  public ProcessDataGeneric(T delegate) {
    this.delegate = delegate;
  }
}

public class ProcessDataMotorferdsel extends ProcessDataCommon {
  public ProcessDataMotorferdsel() {
    unwrapped = new ProcessDataGeneric<ProcessDataMotorferdsel>(this);
  }
}

I get compile error on line: unwrapped = new ProcessDataGeneric<ProcessDataMotorferdsel>(this); Saying

[javac] ProcessDataMotorferdsel.java:52: incompatible types [javac] found : ProcessDataGeneric<ProcessDataMotorferdsel> [javac] required: ProcessData<ProcessDataCommon> [javac]

I cannot make heads or tails of that error message. The ProcessDataMotorferdsel class extends ProcessDataCommon, so IMO it should work.

Answer 1

Maybe I'm missing something too, but

ProcessDataCommon pdc = null;
if(processData.getDelegate() instanceof ProcessDataCommon) {
   pdc = (ProcessDataCommon) processData.getDelegate();
}

should be equivalent..? As you mentioned, the delegate is always ProcessDataCommon type.

If ProcessData#getDelegate() would then return a ProcessDataCommon, you could eliminate the remaining instanceof check too.

Answer 2

This answer is probably over-simplistic, but I think that nearly all of the code in your question would be eliminated by an ideal solution. The problem for people trying to answer the question is that the real problem that the code is trying to solve isn't clear from what is left over.

A common approach to refactoring away instanceof is to use sub-classes in conjunction with a "tell not ask" style of interface. There is no need to ask ProcessDataGeneric for its delegate if you can tell ProcessDataGeneric to do the whole task for you, like this:

public interface ProcessData {
    public <T> T process(Data data);
}

public class ProcessDataGeneric implements ProcessData {
    private ProcessData delegate;

    public ProcessDataGeneric(ProcessData delegate) {
        this.delegate = delegate;
    }

    public <T> T process(Data data) {
        return delegate.process(data);
}

I'm not even sure that you really need ProcessDataGeneric, since all it does is hold the real ProcessData sub-class:

public class ProcessDataMotorferdsel implements ProcessData {

    // Process the data the Motorferdsel way.
    public <T> T process(Data data) { ... }
}

public class ProcessDataTilskudd implements ProcessData {

    // Process the data the Tilskudd way.
    public <T> T process(Data data) { ... }
}

... and then you can use the sub-classes like this:

ProcessData processor = new ProcessDataMotorferdsel();
Result      result    = processor.process(data);

... without having to worry about delegates and what type they are.

It is often better to use a factory class instead of a constructor to obtain the sub-class instance, particularly if the correct sub-class needs to be calculated.

Answer 3

You don't have to cast if your object already extends the desired target class. I mean, you can do this:

public interface ProcessData {
    public void setDelegate(ProcessDataCommon delegate);
    public ProcessDataCommon getDelegate();
}

and this:

public class ProcessDataGeneric implements ProcessData {
    private ProcessDataCommon delegate;
    public ProcessDataGeneric(ProcessDataCommon delegate) {
        this.delegate = delegate;
    }
    @Override
    public ProcessDataCommon getDelegate() {
        return delegate;
    }
    @Override
    public void setDelegate(ProcessDataCommon delegate) {
        this.delegate = delegate;
    }
}

And your instanceof comb simplifies to:

ProcessDataCommon pdc = processData.getDelegate();

Answer 4

I made it work.

Look at UPDATE #2 and including this change:

public abstract class ProcessDataCommon<T extends ProcessDataCommon<?>> implements ProcessDataWrapper {
}

Made everything compile.