Avoid If-else code smell with creation of objects which depend upon specific conditions

Question

Is there a better way to deal with an instanciation of an object (Product) which depends upon another object type (Condition) than using if-else paired with instanceof as the following code shows?

import java.util.ArrayList;
import java.util.List;

abstract class AbstractProduct {
    private AbstractCondition condition;
    public AbstractProduct(AbstractCondition condition) {
        this.condition = condition;
    }
    public abstract void doSomething();
}

class ProductA extends AbstractProduct {
    AbstractCondition condition;
    public ProductA(AbstractCondition condition) {
        super(condition);
    }

    @Override
    public void doSomething() {
        System.out.println("I'm Product A");
    }
}

class ProductB extends AbstractProduct {    
    public ProductB(AbstractCondition condition) {
        super(condition);
    }   

    @Override
    public void doSomething() {
        System.out.println("I'm Product B");
    }
}

class AbstractCondition { }

class ConditionA extends AbstractCondition { }

class ConditionB extends AbstractCondition { }

public class Try {
    public static void main(String[] args) {
        List<AbstractCondition> conditions = new ArrayList<AbstractCondition>();
        List<AbstractProduct> products = new ArrayList<AbstractProduct>();

        conditions.add(new ConditionA());               
        conditions.add(new ConditionB());               
        conditions.add(new ConditionB());               
        conditions.add(new ConditionA());

        for (AbstractCondition c : conditions) {
            tryDoSomething(c);
        }
    }

    public static void tryDoSomething(AbstractCondition condition) {
        AbstractProduct product = null;
        if (condition instanceof ConditionA) {
            product = new ProductA(condition);
        } else if (condition instanceof ConditionB) {
            product = new ProductB(condition);
        }
        product.doSomething();
    }
}

The difference with the code above of my real code is: I have NO direct control over AbstractCondition and its subtypes (as they are in a library), but the creation of a concrete subtype of AbstractProduct depends on the concrete condition.

My goal being: try to avoid the if-else code smell in tryDoSomething().

I would also like to avoid reflection because it feels like cheating and I do think it's not an elegant, clean and readable solution.

In other words, I would like to tackle the problem just with good OOP principles (e.g. exploiting polymorphism) and pheraps some design patterns (which apparently I don't know in this specific case).

you can pass class name as string and use switch statement to avoid if else — shreyansh jogi
– shreyansh jogi, Commented Oct 27, 2016 at 9:46
Add to AbstractProduct a method buildProduct, implements in each class like you want. — AxelH
– AxelH, Commented Oct 27, 2016 at 9:50
switch is not going to work, because basically it's just the same as if-else (with a minor tweak). I would like to exploit the OOP, I prefer not using branching. — 1d0m3n30
– 1d0m3n30, Commented Oct 27, 2016 at 9:50
I would also think about if it is really necessary to mirror each Condition with a Product. Of course it depends on the rest of your code, but duplicating class hirarchies is not so nice in itself. Maybe you can implement it in a diferent way, not creating one Product per Condition but one general Product (probably with some strategies for specific behaviour)!? — Arne Deutsch
– Arne Deutsch, Commented Oct 27, 2016 at 10:15

Naomi · Accepted Answer · 2016-10-27 10:14:55Z

3

Since you can't edit the original objects, you need to create a static map from condition type to product type:

private static HashMap< Class<? extends AbstractCondition>, 
                        Class<? extends AbstractProduct>
                      > conditionToProduct;`

Fill it in static initialization with the pairs of Condition,Product:

static { 
  conditionToProduct.put(ConditionA.class, ProductA.class); 
  ... 
}

and in runtime just query the map:

Class<? extends AbstractProduct> productClass = conditionToProduct.get(condition.getClass());
productClass.newInstance();

edited Oct 27, 2016 at 10:14

answered Oct 27, 2016 at 9:58

Naomi

5,56626 silver badges39 bronze badges

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

2 Comments

Naomi Over a year ago

But I'm not amending a previous approach, I'm offering a totally different solution

AxelH Over a year ago

Because the first one doesn't occurs to answer the question, so edit it instead.

Naomi · Accepted Answer · 2016-10-27 09:47:03Z

2

AbstractCondition needs to know either the type or how to construct a product.

So add one of the following functions to AbstractCondition

Class<? extends AbstractProduct> getProductClass()

or

AbstractProduct createProduct()

answered Oct 27, 2016 at 9:47

Naomi

5,56626 silver badges39 bronze badges

2 Comments

AxelH Over a year ago

I prefer the abstract AbstractProduct createProduct() in the AbstractCondition. Cleaner :)

1d0m3n30 Over a year ago

I can't modify AbstractCondition and its subtypes, they are in a library.

AxelH · Accepted Answer · 2016-10-27 11:33:35Z

0

You should create a Factory class to help you with that then.

interface IFactoryProduct{
    AbstractProduct getProduct(AbstractCondition condition)  throws Exception;
}

This will be your interface, just need to implement it like this.

class FactoryProduct implements IFactoryProduct{

    public AbstractProduct getProduct(AbstractCondition condition) throws Exception{
        return (AbstractProduct)getClass().getMethod("getProduct", condition.getClass()).invoke(this, condition);
    }

    public ProductA getProduct(ConditionA condition){
        return new ProductA();
    }

    public ProductB getProduct(ConditionB condition){
        return new ProductB();
    }

}

Using the reflexion to redirect with the correct method will do the trick. this is upgradable for subclassed if you want.

EDIT:

Some example :

    List<AbstractCondition> list = new ArrayList<AbstractCondition>();
    list.add(new ConditionA());
    list.add(new ConditionB());

    for(AbstractCondition c : list){
        try {
            System.out.println(f.getProduct(c));
        } catch (Exception ex) {
            Logger.getLogger(Main.class.getName()).log(Level.SEVERE, null, ex);
        }
    }

labo.ProductA@c17164

labo.ProductB@1fb8ee3

A more complexe reflexion version allowing a subclass to be received :

public AbstractProduct getProduct(AbstractCondition condition) throws Exception{
    Method m = getMethodFor(condition.getClass());
    if(m == null )
        throw new Exception("No method for this condition " + condition.getClass().getSimpleName());
    else
        return (AbstractProduct) m.invoke(this, condition);
}

private Method getMethodFor(Class<? extends AbstractCondition> clazz ) throws Exception{
    try {
        return getClass().getMethod("getProduct", clazz);
    } catch (NoSuchMethodException ex) {
        if(clazz.getSuperclass() != AbstractCondition.class){
            return getMethodFor((Class<? extends AbstractCondition>)clazz.getSuperclass());
        }
        return null;
    }
}

This allows me to send ConditionC extending ConditionB to build the same product has ConditionB would. Interesting for complexe heritage.

edited Oct 27, 2016 at 11:33

answered Oct 27, 2016 at 10:04

AxelH

14.6k2 gold badges27 silver badges56 bronze badges

7 Comments

1d0m3n30 Over a year ago

It won't work. it will always get into getProduct(AbstractCondition condition) never reaching the other methods in override. That's because getProduct method cares about the static type not the dynamic one. And the static type in my method tryDoSomething() is AbstractCondition.

AxelH Over a year ago

@1d0m3n30 Yes, you are right, I figure that out myself ... I am working on this ^^

AxelH Over a year ago

@1d0m3n30, I knew this was possible, just using some (unsafe) reflection ;)

Michael Gantman Over a year ago

Factory pattern is absolutely right idea for this problem. The implementation in the answer is problematic but Factory that accept Condition argument and returns appropriate subtype of Product is the way to go

AxelH Over a year ago

@MichaelGantman, this is not the cleanest solution, but the factory pattern is not design to work without some condition. get("this") or get("that") will use condition to redirect to the correct method. But I might miss something here ...

|

Collectives™ on Stack Overflow

Avoid If-else code smell with creation of objects which depend upon specific conditions

3 Answers 3

2 Comments

2 Comments

7 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

2 Comments

2 Comments

7 Comments

Your Answer

Sign up or log in

Post as a guest

Related