LinkedList: remove an object

Question

Is this a valid way to find and remove item from a LinkedList in Java using a for each loop, is it possible that inconsistency may arise:

for(ObjectType ob : obList) {
  if(ob.getId() == id) {
    obList.remove(ob);
    break;
   }
}

Answer 1

Others have mentioned the valid point that normally this is not how you remove an object from a collection. HOWEVER, in this case it's fine since you break out of the loop once you remove.

If you want to keep iterating after a remove, though, you need to use an iterator. Otherwise you'll get a ConcurrentModificationException, or in the more general case, undefined behavior.

So yes, if you break out of the foreach after you remove, you'll be fine.

---

To those who's saying that this will fail because you can't modify a collection in a foreach -- this is true only if you want to keep iterating. That's not the case here, so this shortcut is fine.

A ConcurrentModificationException is checked and thrown by the iterator. Here, after the remove (which qualifies as concurrent modification), you break out of the loop. The iterator doesn't even get a chance to detect it.

It may be best if you add a comment on the break, why it's absolutely necessary, etc, because if this code is later modified to continue iterating after a remove, it will fail.

I would treat this idiom similar to goto (or rather, labeled break/continue): it may seem wrong at first, but when used wisely, it makes for a cleaner code.

Answer 2

It is best to use an iterator and use it's remove method when searching for an object by iterating over a collection in order to remove it. This is because

1. The collection could be, for example, a linked list (and in your case it is) whose remove method means searching for the object all over again, which search could have O(n) complexity.
2. You can't continue iteration after the remove unless you use the iterator's remove method. Right now you are removing the first occurrence - in future you might need to remove all matching occurrences, in which case you then have to rewrite the loop.

I recommend, on principle, foregoing the enhanced for and using something like this instead:

for(Iterator<ObjectType> it=obList.iterator(); it.hasNext(); ) {
    if(it.next().getId()==id) { 
        it.remove(); 
        break;
        }
    } 

That way you are not making assumptions about the underlying list that could change in the future.

---

Compare the code to remove the last entry called by the iterator remove (formatting Sun's):

private E remove(Entry<E> e) {
    if (e == header)
        throw new NoSuchElementException();

    E result = e.element;
    e.previous.next = e.next;
    e.next.previous = e.previous;
    e.next = e.previous = null;
    e.element = null;
    size--;
    modCount++;
    return result;
}

against what remove(Object) must do:

public boolean remove(Object o) {
    if (o==null) {
        for (Entry<E> e = header.next; e != header; e = e.next) {
            if (e.element==null) {
                remove(e);
                return true;
            }
        }
    } else {
        for (Entry<E> e = header.next; e != header; e = e.next) {
            if (o.equals(e.element)) {
                remove(e);
                return true;
            }
        }
    }
    return false;
}