0

I used

ArrayList<char[]> record = new ArrayList<char[]>();

to define a arrayList of arrays but when I want refer to elements in this arrayList, I wrote:

for(char[] char1 : record){
    if(Arrays.equals(char1, char2)){
        return false;
    }
    record.add(char2);
}

It didn't work and has the error message like:

java.util.ConcurrentModificationException

in line 

for(char[] char1 : record){

What's wrong with my code? Thank you!

Eleanor
  • 306
  • 1
  • 4
  • 14

2 Answers2

0

Take a look at the javadoc for ArrayList:

The iterators returned by this class's iterator and listIterator methods are fail-fast: if the list is structurally modified at any time after the iterator is created, in any way except through the iterator's own remove or add methods, the iterator will throw a ConcurrentModificationException.

An enhanced for-loop that you've used like on this line:

for(char[] char1 : record){

implicitly creates a list iterator and iterates over it.

The problem arises on the line:

record.add(char2);

where you are modifying the list as you iterate through it, you'll need to use the list's iterator to iterate through and add appropriately.

A possible solution would be to manually retrieve the list iterator and iterate, this will expose access to the add() method which can be used to safety add items to the list as you iterate through. For example:

// Here we manually retrieve the list iterator to work with
ListIterator<char[]> iter = record.listIterator();

// Iterating through the entire list is done via the `hasNext()` method
while(iter.hasNext()){
    char[] char1 = iter.next();

    if(Arrays.equals(char1, char2)){
        return false;
    }

    // Note: the add() method can only be called once per next() method call
    iter.add(char2);
}

Note that once added, the list iterator will eventually iterate into the newly added elements. To prevent this, you can add the new elements to a temporary holding list and then transfer across as appropriate (such as when iteration is complete).

initramfs
  • 8,275
  • 2
  • 36
  • 58
  • Explanation of the problem is ok (more verbose that Jon Skeet's, but is ok). Proposed solution is not. – Luiggi Mendoza May 21 '15 at 17:01
  • @LuiggiMendoza Maybe I'm misunderstanding something here, what is wrong with a proposed solution? The function of the solution here is basically identical to the OP's intent, especially for small trivial issue like this, unless I grossly missed the OP's point. Anyhow, I'll add more justification to why it's needed if that helps. – initramfs May 21 '15 at 17:04
  • The solution is wrong because the `while` will add `char2` many times in case it's not found in the `List`. This means, every time you use this method against a `char[]` that's not in the list, you will increase the size of the list by 2x. – Luiggi Mendoza May 21 '15 at 17:10
  • @LuiggiMendoza I'm mirroring the OP's code here, ignoring the reasoning behind the if condition, I'm assuming that's not part of the issue. Besides, your condition doesn't hold since a no-match char2 will cause the enclosing method to return, aborting any iterating condition, definitely not causing char2 to be added to the list. – initramfs May 21 '15 at 17:13
  • Blindly mirroring code is part of the issue as well... And if you fix the code, this problem won't arise to begin with and there is no need to use the `ListIterator` at all. – Luiggi Mendoza May 21 '15 at 17:13
  • @LuiggiMendoza In that case can you offer insight into the relevance of the if condition to the OPs proposed and immediate problem of `ConcurrentModificationException`. Am I missing something? Can the if condition itself cause an ConcurrentModificationException (ignoring the return condition within the block)? – initramfs May 21 '15 at 17:15
  • I mean: if the `list.add(char2)` were outside the loop, this problem won't have arisen in the first place. – Luiggi Mendoza May 21 '15 at 17:17
  • @LuiggiMendoza You can't guarantee that putting `list.add(char2)` outside the loop is what the OP wants. It sure looks like (incorrect) code to add an element if and only if it's not already in the list, but we can't assume that. In my opinion, that is irrelevant to the primary problem of the code and also doesn't not inform the OP on what he/she is doing wrong. By telling them to simply move the statement out of the loop, you are altering functionality without directly addressing the issue at hand (to modify a loop whilst it's being iterated). – initramfs May 21 '15 at 17:21
  • I can guarantee it. Just look at the code with a little more of sense: if the `char[]` is in the list, return `false`, in order to not allow duplicates. But if the `char[]` isn't stored in the list, then add it. – Luiggi Mendoza May 21 '15 at 17:23
  • @LuiggiMendoza Well, here is where I disagree. I believe no matter how trivial the code looks, there could be stripped complexity for the sake of simplification, unless the OP stated his/her full intention with the code, there is no guarantees. Besides, fixing the OP's problem based on your assumption is not directly answering the question given that the mistake the OP is committing here is a modification of a list whilst iterating. Solving the OP's overall (assumed) problem is unlikely to teach the OP about the specifics of what they are doing wrong. – initramfs May 21 '15 at 17:29
  • For that, there's a dup Q/A that already covers all these details, no need to create more dups in the site. – Luiggi Mendoza May 21 '15 at 17:30
-1

You cannot modify a list while you iterate over it, since this could create an infinite loop with no possibility of escape.

If what you are trying to accomplish is to add n copies of char2 to the end of record where n is the number of arrays char1 not equal to char2 before the first occurrence of char2 in record, then you should simply count how many there are, then add that many copies after the fact or something similar.

ArrayList<char[]> record2 = new ArrayList<char[]>();
for(char[] char1 : record){
    if(Arrays.equals(char1, char2)){
        break;
    }
    record2.add(char2);
}
record.addAll(record2);

The above is what I would do. I instead add to a different list, then after it would have exited, add them all to the end of record.

mbomb007
  • 3,788
  • 3
  • 39
  • 68