Avoiding instanceof in factory method

Question

I'm looking for an improved solution to the following problem. I have an object, which is passed to a factory; the factory will inspect the object type, create another type, which it populates with data from the incoming object, and returns the new one.

...

public MyAbstractClass create(MyObject a) {

    if (a instanceof A) {
        A obj = (A) a;
        return new MyAbstractClass_1 (obj.getField(), factoryField);
    } 
    else if (a instanceof B) {
        B obj = (B) a;
        return new  MyAbstractClass_2 (obj.getSomething(), obj.getSomethingElse(), factoryField);
    } 

}

Instances of the return type are treated generically afterwords. Going forward I need to support more types and if possible I'd like to avoid an instanceof solution. How can I improve this?

joseph · Accepted Answer · 2016-11-11 16:38:24Z

3

Can you add the create method to MyObject instead? That way you won't need instance of anymore because each instance of MyObject knows how to 'create'. You won't have a factory anymore though :(

It would look something like (assuming MyObject is an interface. if it's a class, then just extend instead):

interface MyObject {
  ...
  public MyAbstractClass create(MyObject a);
  ...
}

public class A implements MyObject {
  ...
  public MyAbstractClass create(MyObject a) {
    return new MyAbstractClass_1 (obj.getField(), factoryField);
  }
  ...
}

public class B implements MyObject {
  ...
  public MyAbstractClass create(MyObject a) {
    return new MyAbstractClass_2 (
      obj.getSomething(),
      obj.getSomethingElse(),
      factoryField);
  }
  ...
}

edited Nov 11, 2016 at 16:38

answered Nov 11, 2016 at 16:31

joseph

2,9222 gold badges34 silver badges53 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Dave Cousineau · Accepted Answer · 2016-11-11 21:11:24Z

1

The bodies of your if statements should be virtual or abstract members on MyObject.

abstract class MyObject {
   public abstract MyAbstractClass create();
}

class A extends MyObject {
   @Override
   public MyAbstractClass create(Object factoryField) {
      return new MyAbstractClass_1 (this.getField(), factoryField);
   }
}

class B extends MyObject {
   @Override
   public MyAbstractClass create(Object factoryField) {
      return new  MyAbstractClass_2 (this.getSomething(), this.getSomethingElse(), factoryField);
   }
}

Generally, when you see yourself checking the type of an object to do something different depending on the concrete type, that likely means that you should be using polymorphism and the code should be going into the concrete types themselves.

Updated that the MyObject data should be coming from the current instance and not passed as a parameter, as you pointed out. Only issue is that I'm not sure where you'd put factoryField now. You could pass it as a parameter as above, and since these are virtual members you could still have a factory as well:

class SomeFactory {
   private Object factoryField;

   public SomeFactory(Object factoryField) {
      this.factoryField = factoryField;
   }

   public MyAbstractClass create(MyObject a) {
      return a.create(factoryField);
   }
}

edited Nov 11, 2016 at 21:11

answered Nov 11, 2016 at 16:35

Dave Cousineau

13.4k8 gold badges70 silver badges83 bronze badges

3 Comments

user1491636 Over a year ago

What's the reason for the create method to take any argument? The implementation has what it needs to create the object - except for factoryField.

Dave Cousineau Over a year ago

@user1491636 you're right, that info should be coming from the instance now

user1491636 Over a year ago

You're updated solution is one I was thinking of... not sure if it's really a factory anymore though:)

Collectives™ on Stack Overflow

Avoiding instanceof in factory method

2 Answers 2

Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related