5

I got an issue with deleting an object from ArrayList when working on the assignment If I use the "normal" for loop, it works as following

public void returnBook(String isbn){        
    for (int i = 0; i < booksBorrowed.size(); i++){            
        if (booksBorrowed.get(i).getISBN() == isbn){
            booksBorrowed.get(i).returnBook();
            booksBorrowed.remove(i);                
        }
    }
}

However, when I'm trying to simplify the code with enhanced for-loop, that doesn't work and showing java.util.ConcurrentModificationException error:

public void returnBook(String isbn){        
        for (Book book: booksBorrowed){            
            if (book.getISBN() == isbn){
                book.returnBook();
                booksBorrowed.remove(book);                
            }
        }
}

Hope you guys could lighten me up..

Improve this question
1
  • If your question is "Why do I get an error" it is because you can't remove items from a list you're iterating over. And your first loop may have a bug in it, if the same ISBN can be in the list twice. Commented Apr 1, 2012 at 3:28

5 Answers 5

Reset to default
7

Your alternatives to avoid a ConcurrentModificationException are:

List<Book> books = new ArrayList<Book>();
books.add(new Book(new ISBN("0-201-63361-2")));
books.add(new Book(new ISBN("0-201-63361-3")));
books.add(new Book(new ISBN("0-201-63361-4")));

Collect all the records that you want to delete on enhanced for loop, and after you finish iterating, you remove all found records.

ISBN isbn = new ISBN("0-201-63361-2");
List<Book> found = new ArrayList<Book>();
for(Book book : books){
    if(book.getIsbn().equals(isbn)){
        found.add(book);
    }
}
books.removeAll(found);

Or you may use a ListIterator which has support for a remove method during the iteration itself.

ListIterator<Book> iter = books.listIterator();
while(iter.hasNext()){
    if(iter.next().getIsbn().equals(isbn)){
        iter.remove();
    }
}

Or you may use a third-party library like LambdaJ and it makes all the work for you behind the scenes>

List<Book> filtered = select(books, 
                having(on(Book.class).getIsbn(), 
                        is(new ISBN("0-201-63361-2"))));
Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

4

You really shouldn't be doing either as they will cause problems in the end. Instead use the ArrayList's iterator to help you iterate through the list and then remove only with the iterator. This will help prevent pernicious concurrent modification errors.

Improve this answer

1 Comment

/golfclap use of 'pernicious'
2

All good answers. But I would sugest you to rethink it. I mean, do you really need a ArrayList or a HashMap would be better? If your list of objects have a unic key (ISBN), and you use it to get each object, why not use a collection appropriated for your problem?

You woud do only this

public void returnBook(String isbn){        
     Book book = (Book) booksBorrowed.remove(isbn);            
     book.returnBook();    
}
Improve this answer

Comments

1

You have a bug in your code:

for (int i = 0; i < booksBorrowed.size(); i++){            
    if (booksBorrowed.get(i).getISBN() == isbn){
        booksBorrowed.get(i).returnBook();
        booksBorrowed.remove(i);                
    }
}

It skips next elements after removed ones. E.g. when you removed '0th' element, 1st becomes 0th, but this code doesn't iterate through it.

This is a correct version:

for (int i = booksBorrowed.size() - 1; i >= 0; i--){            
    if (booksBorrowed.get(i).getISBN() == isbn){
        booksBorrowed.get(i).returnBook();
        booksBorrowed.remove(i);                
    }
}

But this is not the best approach, because it's complexity is O(n^2).

A better one is to add all retained items to another collection and then copy them back to the original list with truncating size. It's complexity is O(n). Of course, it's a concern only if there are many elements to remove.

P.S. removing in a for-each construction breaks iterator, so it's not a valid way to process the list in this case.

But you can do the following:

    for (Iterator<String> i = a.iterator(); i.hasNext();) {
        Book next = i.next();
        if (book.getISBN() == isbn){
           book.returnBook();
           i.remove(i);                
        }
    }

Again, the complexity is O(n^2) in this case.

Improve this answer

4 Comments

First loop works if he adds the uber-gross "i--;" at the bottom of the if statement.
In the last one, does it remove from the ArrayList or only from the iterator?
Not sure the first loop is O(n^2). The time to find an remove all ISBNs seems to be O(n).
Removing a single element is O(n) for ArrayList - as the whole array has to be shifted. There are O(n) removal operations.
0

When you are using the enhanced for-loop in Java it uses the Iterator of the list to iterate over the list. When you remove an item with the remove function of the list, that will interfere with the state of the iterator and iterator will throw a ConcurrentModificationException. With the simple for-loop you don't have such problem because you are only using the list and the state change happens only in the list itself.

Improve this answer

2 Comments

Care to enlighten me a bit with how to use the iterator to remove Book object
it really depends on what is you application and what kinda performance you want. A combination of hashmap and arraylist iterator will do that.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.