Search code examples
javaclone

The best practice using clone in an aggregated class


I have a system where there is a Customer with an ArrayList of supplements as its attribute.

The code for Customer class is :

public class Customer implements Cloneable
{
         .
         .
         ArrayList<Supplement> suppList
    public Customer(String fName, String lName, String emailInput, ArrayList<Supplement> list)
    {
        setFName(fName);
        setLName(lName);
        setEmailAddr(emailInput);
        setSuppList(list);
    }
   public void setSuppList(ArrayList<Supplement> list)
    {
        suppList = new ArrayList<Supplement>();
        for(Supplement sp : list)
        {
            suppList.add(sp);
        }
    }
}
 public ArrayList<Supplement> getSuppList() throws CloneNotSupportedException
    {
        ArrayList<Supplement> list = new ArrayList<Supplement>();
        if (suppList != null)
        {
            for(Supplement sp : suppList)
            {
                list.add((Supplement)sp.clone());
            }
        }
        return list;
    }
 public void addSupp(Supplement item)
    {
        suppList.add(item);
    }

    public void removeSupp(Supplement item)
    {
        suppList.remove(item);
    }

Initially, my setSuppList method just contain one line of code which is suppList = list and my getSuppList method is just 'return suppList'. I felt like it is a privacy leak so I called cloning on both methods. For setSuppList, suppList = new ArrayList() and it iterates through the argument list and clone each object and add it inside the suppList array. For getSuppList, it iterates through suppList and clone every Supplement object inside it, add it into a new array and return the array. However, I realize if changing a supplement object price from $3 to $50, and if I have 100 customers, means I have to keep calling setSuppList() 100x.

I changed my mind, so I change setSuppList method to suppList = list and keep the cloning stuff only in getSuppList.

Then it comes to my mind... why dont I make suppList as a new array in setSuppList and add every item from the argument 'list' to the suppList. That way, both list and suppList refer to the same objects but when list removes an item, the suppList of the Customer object remains unaffected. Is just when the individual item is affected, the suppList updates accordingly (e.g. the price of the supplement) and when I want to add or remove an item in suppList, I can use addSupp method or removeSupp method, or setSuppList method.

public static void main(String[] args)
    {
        try
        {
            Supplement s1 = new Supplement("A", 2.9);
            Supplement s2 = new Supplement("B", 3);
            ArrayList<Supplement> spList = new ArrayList<Supplement>();
            spList.add(s1);
            spList.add(s2);
            Customer cstmr = new Customer("killua", "zoldyck", "killua@gmail.com", spList);



ArrayList<Supplement> spList2 = cstmr.getSuppList();
        System.out.println("cloned array size : "+spList2.size());
        spList2.remove(0);
        System.out.println("After removing an item in the cloned array");
        System.out.println("cloned array size : "+spList2.size());
        System.out.println("Array returned from getSuppList size : "+cstmr.getSuppList().size());

    spList.remove(0);
    System.out.println("array size : "+cstmr.getSuppList().size());
    System.out.println("");

    spList.get(0).setWeeklyCost(50);
    System.out.println("If I change the first item in the first array prize to 50");
    System.out.println("price of 1st item in object in the first array : "+spList.get(0).getWeeklyCost());
    System.out.println("price of 1st item in object in the array returned by getSuppList : "+cstmr.getSuppList().get(0).getWeeklyCost());
    System.out.println("");

    s1.setWeeklyCost(40);
    System.out.println("If I change the supplement object directly to 40");
    System.out.println("price of 1st item in object in the first array : "+spList.get(0).getWeeklyCost());
    System.out.println("price of 1st item in object in the array returned by getSuppList : "+cstmr.getSuppList().get(0).getWeeklyCost());

}
catch(CloneNotSupportedException e)
{

}

}

Output

cloned array size : 2

After removing an item in the cloned array

cloned array size : 1

Array returned from getSuppList size : 2

array size : 2

If I change the first item in the first array prize to 50

price of 1st item in object in the first array : 50.0

price of 1st item in object in the array returned by getSuppList : 2.9

If I change the supplement object directly to 40

price of 1st item in object in the first array : 50.0

price of 1st item in object in the array returned by getSuppList : 40.0

However, when I tried this, there is an unexpected behaviour. spList.get(0).setWeeklyCost(50); does not affect the suppList object of a Customer object.

How can that be? both spList and suppList refer to the same objects although they are different arrays...


Solution

  • Since Supplement is its own thing, entirely different from Customer, and can be renamed and repriced by itself, you should never clone it as part of Customer logic.

    You should "clone" the list, since that is a property of the customer, which is easily done by calling the copy-constructor:

    suppList = new ArrayList<>(list);
    

    spList.get(0).setWeeklyCost(50); does not affect the suppList object of a Customer object. How can it be ??

    Because you are still cloning Supplement when you call cstmr.getSuppList()