10
\$\begingroup\$

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<Vegetable> buy(int num, Class vegetableClass)
    {
        ArrayList<Vegetable> res = new ArrayList<Vegetable>(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.

\$\endgroup\$
  • \$\begingroup\$ I would use dictionary instead of switch look at this question my answer Factory design pattern \$\endgroup\$ – Serghei Jan 31 '12 at 9:21
  • \$\begingroup\$ I take exception to both tomatoes and cucumbers being classified as vegetables ;-) \$\endgroup\$ – rolfl Jan 19 '14 at 18:27
14
\$\begingroup\$

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<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;
 }

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<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;
 } 

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.

\$\endgroup\$
6
\$\begingroup\$

Is it a good way of doing that?

No. Give more thought about the separation of concerns for each class. The buy logic does not belong in a factory. A factory is only for creating the various veggie types. Buy should be in a VegetableStand class or perhaps in the Vegetable class.

If "buying" logic is unique per vegetable then put buy() in the Vegetable class - especially if it also has a price property. Even so, I can see a VegetableCart.Buy() that accumulates everything into a "bag" so the different veggies, quantities, and prices can be added - for a total price to the customer.

And is it ideologically correct to name it factory?

Not the way you've written it. A veggie factory is for creating various veggies. I expect to see a create() method, passing in an enum as suggested above.

public class VegetableStand {
    VeggieFactory myVF = new VeggieFactory();

    public ArrayList<Vegetable> bag = new ArrayList<Vegetable>;

    public ArrayList<Vegetable> Buy (VegetableType whatKind ,int howMany){
        for (int i=0, i<howMany; i++) {
            bag.add(myVF.Create(whatKind);
        }
    }

    public double TotalSale (ArrayList<Vegetable> bag) {
        double totalPrice = new double;

        foreach (Vegetable item in Bag) {
             totalPrice += item.Price;
         }

       return totalPrice;
    }
}

internal class VeggieFactory {
    public static Vegetable Create(VegetableType whatKind) {
        Vegetable thisKind;

        switch(whatKind) {
            case: VegetableType.carrot
                thisKind = CreateCarrot();
                break;
            case: VegetableType.cucumber
                thisKind = CreateCucumber();
                break;
            default:
               throw new Veggie not invented yet exception;
        }
        return thisKind;
    }
}

Then, in the buy() method you should not have a big switch statement in the for loop. If you're passing in a veggie instance, that doesn't make sense. If you already created that veggie, why the factory?

  • You should separate the creation of the ArrayList<Vegetable> from the creation of the vegetables themselves. The buy() method creates a new list for every veggie you buy. And, you may want to instantiate a veggie w/o a list.
\$\endgroup\$
  • \$\begingroup\$ Would it make sense in this case you pass the VeggieFactory into the VegetableStand via the constructor to decouple them? \$\endgroup\$ – Supericy Jan 23 '13 at 0:31
  • \$\begingroup\$ @Supericy, doesn't hurt, if I expected the factory design to evolve such that there will be various kinds of VeggieFactory then absolutely. \$\endgroup\$ – radarbob Feb 23 '13 at 17:55
3
\$\begingroup\$

As far I see nobody pointed out yet that the reference type of the list should be only List. Instead of

ArrayList<Vegetable> res = new ArrayList<Vegetable>(num);

use

List<Vegetable> res = new ArrayList<Vegetable>(num);

The signature of the method also should be changed:

public static List<Vegetable> buy(int num, Class vegetableClass)

Type List vs type ArrayList in Java

\$\endgroup\$
2
\$\begingroup\$

Apart from what's already been said, when dealing with Class, you should use type boundaries to eliminate the possibility of receiving something you weren't expecting:

public static <T extends Vegetable> ArrayList<Vegetable> buy(int num, Class<T> vegetableClass)

And then:

v = (T) vegetableClass.getDeclaredConstructor(Double.TYPE).newInstance(weight);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.