How to clone the List correctly in the constructor?

Question

The constructor takes a List as parameters, this List needs to be filled with the same elements, for example "1" and cloned 7 times, assign the result to different collections, so that then 7 methods would process each of their lists.

constructor call:

public Class1 {
    public Class2 arrayList = new Class2(new ArrayList<>(10000))
    public Class2 linkedList = new Class2(new LinkedList<>())
    public Class2 cowList = new Class2(new CopyOnWriteArrayList<>())
}

public Class2 {

    private List<Integer> list;
    private List<Integer> list2;
    private List<Integer> list3;
    private List<Integer> list4;
    private List<Integer> list5;
    private List<Integer> list6;
    private List<Integer> list7;

    public Class2(List<Integer> list) {

        for (int i = 0; i < list.size(); i++) {
            list.add(1);
        }

        this.list = list;
        this.list2 = list;
        this.list3 = list;
        this.list4 = list;
        this.list5 = list;
        this.list6 = list;
        this.list7 = list;
    }

    method1(){
        // list - doSomething...;
    }

    method2(){
        // list1 - doSomething...;
    }

    method3(){
        // list2 - doSomething...;
    }    

    method4(){
        // list3 - doSomething...;
    }

    ...

}

Each method performs a specific operation on its list. Depending on the constructor being called, the list is - ArrayList or LinkedList or CopyOnWriteArrayList

My code doesn't work. What am I doing wrong?

Answer 1

With the below statements, all your lists point to the same memory location and hence any operation on the list would be visible to all of list1 to list7.

this.list = list; this.list2 = list; this.list3 = list;

Instead you need to generate a new arraylist containing the elements of the input list using the appropriate list constructor which accepts a Collection as the argument.

Since the input could be of any List type, you could make use of instanceof to check the type of input and invoke the appropriate constructor.

NOTE: You are only creating a new list. So changes to the original list won't be reflected on those seven lists. BUT the objects contained in the list are still referenced in the new lists as we are not cloning the object. We are just creating a new list with the same objects. SO if you were using List<SomeObject> instead of List<Integer>, were SomeObject is mutable, then any operations on the objects would be reflected on every list. This is not applicable to this specific question as it uses immutable Integer objects.

private static List<Integer> copy(List<Integer> input) {
    if(input instanceof ArrayList) {
        return new ArrayList<>(input);
    } else if (input instanceof LinkedList) {
        return new LinkedList<>(input);
    }
    // others
    ...............
    ...............

}

Now invoke this method in your class' constructor.

this.list = copy(list);
this.list2 = copy(list);
this.list3 = copy(list);

You could also replace the for loop at the beginning with the below:

Collections.fill(list, 1);

But with the current code, all inputs are empty and hence the size of every list you are passing would be 0. Hence no filling of elements would take place (using for loop or Collections.fill)

new ArrayList<>(10000) does not update the size of the arraylist. It only initializes the backing array.

Read this too.