CODEWITHSUNDEEP
javaloopsforeachconcurrencyiterator
Bradley
Bradley

Reputation: 337

ConcurrentModificationException whilst removing element using nested for-each

CSV Input File.

EmployeeID,FirstName,LastName,Dept
1, John, Smith, Maintenance 
1, John, Smith, Engineering 
1, John, Smith, Transport

From the CSV file I read each line and create an employee object which is then stored into an ArrayList of Employee's.

Before sorting employee's:

employeeID='1', firstName='John', lastName='Smith', department='Maintenance'}
employeeID='1', firstName='John', lastName='Smith', department='Engineering'}
employeeID='1', firstName='John', lastName='Smith', department='Transport'}

Once file processing has finished, I move onto sorting the Employee’s.

Desired output after sort:

employeeID='1', firstName='John', lastName='Smith', department='Maintenance, Engineering, Transport'}

Using the For-Each with a nested For-Each, I compare the EmployeeID’s, if a match is found I append the dept of the duplicate to the original and remove the duplicate. Obviously as I’m modifying the list I’m iterating over I then get a ConcurrentModificationException. 

Exception in thread "main" java.util.ConcurrentModificationException
    at java.base/java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1042)
    at java.base/java.util.ArrayList$Itr.next(ArrayList.java:996)
    at com.gillott.csvtomap.CsvToMap.employeeSorter(CsvToMap.java:73)
    at com.gillott.csvtomap.CsvToMap.main(CsvToMap.java:34)

I’ve seen online the advice for carrying out modification on the collection you are iterating over is to use the Iterator, I’ve implemented the Iterator but still get the same results, Is this because I’m using a nested For-Each? See For-Each implementation and For-Each with Iterator implementation.

  1. For-Loop example:

    for (Employee emp1: employees) {

    for (Employee emp2: employees){

        if(emp1.getEmployeeID().equals(emp2.getEmployeeID())){

            emp1.setdepartment(emp1.getdepartment() + ", " + emp2.getdepartment());
            System.out.println("Duplicate found for Employee ID: " + emp1.getEmployeeID());
            employees.remove(emp2); //CME Exception Throw here.
        }
    }
}

  2. Iterator For-Loop example:

    for (Iterator<Employee> employeeIterator1 = employees.iterator(); employeeIterator1.hasNext();) {
    Employee emp1 = employeeIterator1.next();

    for (Iterator<Employee> employeeIterator2 = employees.iterator(); employeeIterator2.hasNext();){
        Employee emp2 = employeeIterator2.next();

        if(emp1.getEmployeeID().equals(emp2.getEmployeeID())){

            emp1.setdepartment(emp1.getdepartment() + ", " + emp2.getdepartment());
            System.out.println("Duplicate found for Employee ID: " + emp1.getEmployeeID());
            employeeIterator1.remove();
        }
    }
}

My implementation of the Iterator could be incorrect, this is the first time using the Iterator directly.

Upvotes: 0

Views: 62

Answers (3)

AnonymousFox
AnonymousFox

Reputation: 118

Ideally, if implemented right, the iterator way should work. However, the issue with the code you've written in 'iterator for-loop' case is that you are initializing two iterators for the same List (employees) i.e both

Iterator employeeIterator1 = employees.iterator();

Iterator employeeIterator2 = employees.iterator();

both iterators are created for same list. For this reason, ConcurrentModificationException is thrown by the employeeIterator2, which indicates that the list has been changed (except by the Iterator's own remove method, in your case by 'employeeIterator1') after the Iterator employeeIterator2 was created.

For your scenario, you can create a dummy/temporary list which is copy of the original employee list and use that new list while creating the iterator2.

Upvotes: 0

Andy Turner
Andy Turner

Reputation: 140318

Group the employees by id, and use a downstream collector to merge the instances:

Map<Integer, Employee> map =
    employees.stream()
        .collect(groupingBy(
            Employee::getId,
            reducing((a, b) -> a.setdepartment(a.getdepartment() + "," + b.getdepartment()));

Then retain the instances in employees which are values in this map:

employees.retainAll(map.values());

You can do this all in a single statement, if you so desire.

Upvotes: 2

0xh3xa
0xh3xa

Reputation: 4857

Because you loop over the array and remove it, you can use another list that contains duplicates and remove from it or use any Concurrent list like CopyOnWriteArrayList

You can remove the duplicates using Set, and override the equals() and hashCode() in Employee class and determine which attributes to be used in hashCode and equals

Set<Employee> unique = new HashSet<>(employees);

More information: Concurrent collection Oracle doc

Upvotes: 1

Related Questions