Java design issue - Adding functionality to fixed classes

Question

I have a set of classes that all need to be acted on in some (individual) way.

Ordinarily I'd just create a DoSomethingInterface with a single doSomething() method and have each class implement that method in a way that suits each class' needs. However, I cannot do that in this case as the class definitions are unmodifyable (auto-generated)

So, I reckon I need to create a set of different classes that each take one of the autogenerated classes and performs the operation on them. So, say I have 2 autogenerated classes, Class1 and Class2, I will first define a common Operator interface:

public interface Operator <TYPE>{
  public void doSomething(TYPE type);
}

and then implement one of these per class

public class Class1Operator implements Operator<Class1>{
    public void doSomething(Class1 type){
      ...
      ...
    }
}

and

public class Class2Operator implements Operator<Class2>{
    public void doSomething(Class2 type){
      ...
      ...
    }
}

Ok, so far so good. Now, given that I have an object of type Class1, is there any way of getting its operator without resorting to:

public Operator getOperator(Object obj){
  if(obj instanceof Class1){
    return new Class1Operator();
  }else if(obj instanceof Class2{
    return new Class2Operator();
  }
  return null;
}

Which kinda seems like bad practice to me...

The only other way I can think of is by creating a map of operators to class names like so:

Map<Class, Operator> allOperators = new HashMap<Class, Operator>();
allOperators.put(Class1.class, new Class1Operator());
allOperators.put(Class2.class, new Class2Operator());

and then return the operator using:

public Operator getOperator(Object obj){
  return allOperators.get(obj);
}

But this doesn't seem right (I'm not sure, are there any issues with keying an object off its class....)

Any input as to whether either of these approaches is 'correct'? or is there a more elegant solution??

Thanks

Answer 1

What you've implemented (the map-by-class approach) is one of the alternatives to the GoF Visitor pattern I talk about when I teach patterns. It's efficient and extendable, even at runtime. Much better than the if/else if/else hardwired approach.

The only issue with keying off the class is if the actual instances implement subtypes rather than the class type you mention; then the lookup map won't work.

If you need subtypes to be recognized, I'd recommend Aaron's approach (walk up the superclass chain), but you may also want to look at implemented interfaces as well. If you just need "exact class match", keep your getOperator simple.

Note that you have a bug in getOperator -- it should look as follows:

public Operator getOperator(Object obj){
    return allOperators.get(obj.getClass());
}

One more thing... Hide your map inside another class and manage it as follows:

private Map<Class<?>, Operator<?>> map = new HashMap<Class<?>, Operator<?>>();
public <T> void register(Class<T> clazz, Operator<T> operator) {
    map.put(clazz, operator);
}

This prevents anyone from registering an operator that won't work against the class it's keyed against. (You might want to use Operator as the parameter to allow an operator that's written against a superclass, but that's might not be needed)

Answer 2

You can us Class.isAssignableFrom to get around the sub-typing issue. I use this all the time and while it is not "visitor" elegant it is quite fine in practice.

Answer 3

Would it be possible to create your own class that extends the generated class and then have your class implement the interface?

Answer 4

One of the issues with building a map is that it will not support subclasses unless you register them specifically or extend your get function to look up super classes specifically.

That is to say if B inherits from A and you've registered an operator with A.class. Fetching an operator with B.class will fail, unless you change your getOperator to something like:

public Operator getOperator(Object obj){
  Class<?> current = obj.getClass();
  Operator op;

  while((op = allOperators.get(current)) == null){
    current = current.getSuperclass();

    if(current == null){
      /* 
       * We've walked all the way up the inheritance hierarcy
       * and haven't found a handler. 
       */
      return null;
    }
  }

  return op;
}

Once you've got a reasonable getOperator implementation, mapping classes to operators seems like a reasonable approach.

Answer 5

I'm going to say it's not possible to do just using the interface itself, based on the way Java handles generics.

In Java, generics are erased at compile time and replaced with casts.

I haven't actually checked how it works internally, but at a guess, your interface turns into this:

public interface Operator {
  public void doSomething(Object type);
}

and where it's called, into this:

public class Class1Operator implements Operator{
    public void doSomething(Object type){
      Class1 oType = (Class1) type;
      ...
      ...
    }
}

This still isn't exactly right as type will be cast after it's returned as well, plus Java bytecode doesn't actually look like Java, but you might get the general idea.

The instanceof and Map methods should work, even if they are a bit messy.

Answer 6

Have you considered this:

public Interface Operator {
    public void doSomething();
}

public class Class1Operator extends Class1 implements Operator {
    ...
}

public class Class2Operator extends Class2 implements Operator {
    ...
}

But with reference to your second question of getting an operator to an object without really needing to do the "instanceof" mojo (I guess that's what is looking unclean):

I would suggest that if you can't modify your classes to your exact needs, write a wrapper around them:

public Interface Operator<T> {
    public void doSomething(T obj);
}

public Interface WrappedObject<T> {
    public Operator<T> getOperator();
}
public class WrappedClass1 extends Class1 implements WrappedObject<Class1> {
    ...
}
public class WrappedClass2 extends Class2 implements WrappedObject<Class2> {
    ...
}
public class Class1Operator implements Operator<Class1> {
    ...
}
public class Class2Operator implements Operator<Class2> {
    ...
}

Would that suffice your needs?

Its always a good practice to write wrappers around classes that don't match your needs perfectly, and can't be controlled by you. It helps you keep your code healthy even if these wild classes change.

Cheers,

jrh.