-1

I have an application with custom list class. When trying to do a foreach function with customer argument following happens:

Important! I cannot modify code in main

Main:

XList<Integer> lmod = XList.of(1,2,8, 10, 11, 30, 3, 4);
lmod.forEachWithIndex( (e, i) -> lmod.set(i, e*2));
System.out.println(lmod);
lmod.forEachWithIndex( (e, i) -> { if (i % 2 == 0) lmod.remove(e); } );
System.out.println(lmod);
lmod.forEachWithIndex( (e, i) -> { if (i % 2 == 0) lmod.remove(i); } );
System.out.println(lmod);

XList class:

public class XList <T> extends ArrayList<T> {
public XList(Collection<T> collection) {
    super(collection);
}

public XList(T... ints) {
    super(Arrays.asList(ints));
}

public static <T> XList<T> of(Set<T> set) {
    return new XList<>(set);
}

public static <T> XList<T> of(T... ints) {
    return new XList<>(ints);
}

public void forEachWithIndex(BiConsumer<? super T, ? super Integer> consumer) {
    Iterator<T> iterator = this.iterator();

    int counter = 0;

    while (iterator.hasNext()) {
        consumer.accept(iterator.next(), counter);
        counter++;
    }
}

Error:

Exception in thread "main" java.util.ConcurrentModificationException
at java.base/java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1013)
at java.base/java.util.ArrayList$Itr.next(ArrayList.java:967)
at zad1.XList.forEachWithIndex(XList.java:126)
at zad1.Main.main(Main.java:89)
mattonaly
  • 3
  • 2
  • You're modifying the underlying arraylist (removing elements from it) while iterating over it. That's called a concurrent modification. You just have to not use the iterator in your `forEachWithIndex()` method. And that means no using the enhanced for loop either, as that is basically just syntax sugar for using the iterator. – Charlie Armstrong Oct 30 '21 at 10:22
  • @Progman not really. – mattonaly Oct 30 '21 at 10:38
  • @CharlieArmstrong so what are my options? How to modify the forEachWithIndex method? – mattonaly Oct 30 '21 at 10:38
  • @mattonaly What about [ArrayList's `removeif(Predicate)`](https://docs.oracle.com/javase/8/docs/api/java/util/ArrayList.html#removeIf-java.util.function.Predicate-) method which you already inherit? Something like `lmod.removeIf(i -> i % 2 == 0);`. Oh, didn't read carefully enough. You want to remove by index and not by the respective entry value – Roman Vottner Oct 30 '21 at 10:58

1 Answers1

0

ConcurrentModificationException means:

  1. At point in time A, you make an iterator of some collection either by invoking its .iterator() method, or having a for (var x : collection) {} call it for you.
  2. At point in time B, you change the collection (and not via the iterator you made in A's .remove() method), for example by invoking remove or add or clear or retainAll.
  3. At point in time C, you so much at look funny at that iterator: You invoke any of its methods, or you have the for loop do it by hitting the } of its block.

What you need to do is decidedly nontrivial!

Think about it, given an initial list of [A, B, C, D, E]: you'd presumably want that forEachWithIndex method to get run 5 times, regardless of what happens to the list in between: [0, A], [1, B], [2, C], [3, D], and [4, E]. So what should happen if, during the loop for [0, A], you remove C?

There's an argument to be made that the [2, C] event shouldn't happen at all, and, in fact, that the remaining loops should be [1, B], [2, D], and [3, E]. It's because this is so hard to answer that java solved the problem in the iterator() API by simply not allowing you to do it!

Similar question comes up when you call .add("F") during the loop for [0, A]. Should the for loop run the lambda once with arguments [5, F], or not? An open question.

It's up to you to answer this question, and you should document this in excruciating detail. No matter which choice you make it'll be quite difficult!

I think the for loop should include the changes

This is incredibly complicated. Because imagine that the loop for C ends up removing A. That means that your list will first invoke the lambda with arguments [0, A], [1, B], and [2, C], and then, what's the next iteration going to look like? Presumably the only sane answer then is [2, D]. To make this work you need to track all sorts of things - the list's loop code needs to be aware that deletions happened and that therefore it needs to 'down-adjust' (because you can't simply loop from 0 to 'list size', if you did that, the next iteration would be [3, E] and you have skipped D entirely even though it is still in that list.

Make some coffee, dig in, find a whiteboard, sketch this out. Reserve a full day and be aware that the code will be many pages to deal with it all. Make a TON of test cases and describe in detail precisely what you expect to happen for all of these.

Hmm, okay, nevermind. Let's say it should iterate for all elements the original had no matter what changes

This is easier but inefficient. The fix is simple: First make a copy of the list. Then iterate the copy. The copy cannot change (you're the only one with a ref to it), so they can do whatever they want to the underlying list:

XList<T> copy = new XList<T>(this);
int counter = 0;

var iterator = copy.iterator();
while (iterator.hasNext()) consumer.accept(iterator.next(), counter++);
rzwitserloot
  • 85,357
  • 5
  • 51
  • 72
  • Thank you very much for the in depth explanation! – mattonaly Oct 30 '21 at 11:42
  • I agree with most of this answer, but I'm not sure I agree with your "I think the loop should include the changes" section. You seem to be telling the OP to give up before trying. Sure, it is much more complicated, nobody is going to argue that. But the way you've worded it bothers me because it reads like "this is just beyond you, you should give up on it." – Charlie Armstrong Oct 31 '21 at 02:55
  • @CharlieArmstrong expectation management; if they think it's something that an experienced programmer ought to slap together in 30 minutes, they'll be thinking there is some easy way to do it that they just aren't aware of and will therefore be looking to ask (e.g. right here). I'm trying to indicate that, nope, it'll be hard for anybody: Write out many cases and how you think it should behave, then think about an algorithm that does all that, etc. If they don't think they can do that, it _is_ beyond them. – rzwitserloot Oct 31 '21 at 13:36
  • @rzwitserloot Fair enough. – Charlie Armstrong Oct 31 '21 at 20:00