HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Vegetable factory

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
factoryvegetablestackoverflow

Problem

I'm new to Java. I'm trying to build a simple hierarchy of classes.

abstract class Vegetable
{
    public Vegetable(double weight)
    {
        this.weight = weight;
        ...
    }

    ...
}

class Tomato extends Vegetable
{
    public Tomato(double weight)
    {
            super(weight);
        ...

    }
    ...
}

class Cucumber extends Vegetable
{
    public Cucumber(double weight)
    {
            super(weight);
        ...

    }
    ...
}


I have some abstract methods in class Vegetable, which are overridden in Tomato and Cucumber.
I want to implement logic of buying vegetables. I've created the following class for it:

public class VegetableFactory
{
    public static ArrayList buy(int num, Class vegetableClass)
    {
        ArrayList res = new ArrayList(num);
        for (int i = 0; i < num; i++)
        {
            double weight = (Math.random() * 200) + 100;
            Vegetable v = null;
            try
            {
                v = (Vegetable) vegetableClass.getDeclaredConstructor(Double.TYPE).newInstance(weight);
            } catch (InstantiationException e)
            {
                e.printStackTrace();
            } catch (IllegalAccessException e)
            {
                e.printStackTrace();
            } catch (InvocationTargetException e)
            {
                e.printStackTrace();
            } catch (NoSuchMethodException e)
            {
                e.printStackTrace();
            }
            res.add(v);
        }
        return res;
    }
}


Is it a good way of doing that? And is it ideologically correct to name it factory? Any suggestions of improvement are highly appreciated.

Solution

I would use an enum type instead of having to pass a class in. This way, it's impossible to pass something bad in, and you can catch errors at compile time instead of runtime.

enum VegetableType { CARROT, TOMATO, CUCUMBER };


Then your buy method would be simply

static ArrayList buy(int num, VegetableType vegType) {
    ArrayList veggies = new ArrayList(n); //n is the initial capacity - size is still zero.
    for(int i=0; i < num; ++i) {
        double weight = (Math.random() * 200) + 100;

        switch(vegType) {
        case VegetableType.CARROT:
            veggies.add(new Carrot(weight));
            break;
        case VegetableType.TOMATO:
            veggies.add(new Tomato(weight));
            break;
        case VegetableType.CUCMBER:
            veggies.add(new Cucumber(weight));
            break;
        }
    }
    return veggies;
 }


Now of course the problem is that you end up having a big switch statement in the middle in your loop. Generally, while ugly I'd say that this is sufficient - you get some more control over each of the individual constructors, so you're not constrained to the same set of parameters.

The other thing that you could do is have a factory method on the VegetableType to create an instance:

enum VegetableType {
    CARROT { Vegetable createInstance(int weight) { return new Carrot(weight); }},
    TOMATO { Vegetable createInstance(int weight) { return new Tomato(weight); } },
    CUCUMBER { Vegetable createInstance(int weight) { return new Cucumber(weight); } };

    abstract Vegetable createInstance(int weight);
}


Then your list becomes

static ArrayList buy(int num, VegetableType vegType) {
    ArrayList veggies = new ArrayList();
    for(int i=0; i < num; ++i) {
        double weight = (Math.random() * 200) + 100;
        veggies.add(vegType.createInstance(weight));
    }
    return veggies;
 }


Finally, if you want to get fancy, java.lang.reflect has the Constructor class:

http://docs.oracle.com/javase/1.5.0/docs/api/java/lang/reflect/Constructor.html

You can then use this to get the constructor to the Vegetable type of your choice, and then call construct repeatedly in the loop.

Code Snippets

enum VegetableType { CARROT, TOMATO, CUCUMBER };
static ArrayList<Vegetable> buy(int num, VegetableType vegType) {
    ArrayList<Vegetable> veggies = new ArrayList<Vegetable>(n); //n is the initial capacity - size is still zero.
    for(int i=0; i < num; ++i) {
        double weight = (Math.random() * 200) + 100;

        switch(vegType) {
        case VegetableType.CARROT:
            veggies.add(new Carrot(weight));
            break;
        case VegetableType.TOMATO:
            veggies.add(new Tomato(weight));
            break;
        case VegetableType.CUCMBER:
            veggies.add(new Cucumber(weight));
            break;
        }
    }
    return veggies;
 }
enum VegetableType {
    CARROT { Vegetable createInstance(int weight) { return new Carrot(weight); }},
    TOMATO { Vegetable createInstance(int weight) { return new Tomato(weight); } },
    CUCUMBER { Vegetable createInstance(int weight) { return new Cucumber(weight); } };

    abstract Vegetable createInstance(int weight);
}
static ArrayList<Vegetable> buy(int num, VegetableType vegType) {
    ArrayList<Vegetable> veggies = new ArrayList<Vegetable>();
    for(int i=0; i < num; ++i) {
        double weight = (Math.random() * 200) + 100;
        veggies.add(vegType.createInstance(weight));
    }
    return veggies;
 }

Context

StackExchange Code Review Q#8467, answer score: 14

Revisions (0)

No revisions yet.