Why isn't this for loop working?

Question

So I have to make a class called mylistofstrings that is exactly what it sounds like, an array of strings. One of the methods I have to write is a retain all method, which retains only the strings in the list that are equal to the string entered as a parameter. The for loop seems to skip half of the things it is supposed to remove, any ideas why? The size method just returns how many elements are in the list and is to simple to post.

public boolean retain(String string) {
     if (string == null) {
        throw new NullPointerException();
     }
     MyListOfStrings temp= new MyListOfStrings(this);
     int t=this.size();
     for (int i=0;i<this.size;i++){
         if (string.equals(temp.get(i))!=true){
             this.remove(i);
         }
     }
     return t<this.size();

Here's the get method:

public String get(int index) {
    if (index < 0 || index >= size) {
        throw new IndexOutOfBoundsException(outOfBoundsMsg(index));
    }
    return strings[index];
}

and the remove method:

public String remove(int index) {
    if (index < 0 || index >= size) {
        throw new IndexOutOfBoundsException(outOfBoundsMsg(index));
    }
    String temp = strings[index]; // Save to return at end
    System.arraycopy(strings, index + 1, strings, index, size - index - 1);
    strings[size - 1] = null;
    size--;
    return temp;
}

possible duplicate of [Delete data from ArrayList with a For-loop](http://stackoverflow.com/questions/10738634/delete-data-from-arraylist-with-a-for-loop) — Uwe Plonus, Sep 24 '14 at 14:33

score 3 · Answer 1 · edited May 23 '17 at 10:33

3

You are changing the contents of the array while iterating over it. When removing an item at position x, the item at position x+1 will get the new position x, but since x has already been visited by the loop, the next iteration will be position x+1, and the item that now holds position x will be skipped.

You need to do something like shown here: removing items from list in java

I.e. implement Iterable, and remove items using an Iterator

edited May 23 '17 at 10:33

Community

1
1

answered Sep 24 '14 at 14:32

Tobb

11,850
6
52
77

But don't I fix this by making the new temp class and iterating over that? – trosy Sep 24 '14 at 14:32
No, because you are using the same index in both the temp one and the actual one. As soon as an element is removed, these will be out of sync (so index x in temp is not necessarily the same element as index x in this.) – Tobb Sep 24 '14 at 14:35
As I've mentioned in my answer, I think the misunderstanding is that the OP thinks `new MyListOfStrings(this)` creates a copy of the list, and that in reality it doesn't. We need to see the constructor of `MyListOfStrings` to know for sure. – Rajesh J Advani Sep 24 '14 at 14:42
That might be the case yes, depending on the constructor of course. But anyways, removing stuff from a list while iterating it is probably not the best idea.. – Tobb Sep 24 '14 at 14:43

khelwood · Answer 2 · 2014-09-24T14:44:52.420

Once you start altering one of your containers, they're no longer parallel, so you're not removing the right elements.

You could do it like this:

int i = 0;
while (i < this.size) {
    if (!string.equals(strings[i])) {
         remove(i);
    } else {
         ++i;
    }
}

Note that when you remove element i, the following elements are moved down, and the next element is now at position i, so if you increment i you will go past it.

score 0 · Answer 3 · answered Sep 24 '14 at 14:33

You shouldn't call editing operations during iteration. Consider this list:

1: "1"
2: "2"
3: "3"
4: "4"

Now you want to retain "1" and start to iterate:

Step one:

1: "1" <- do nothing
2: "2"
3: "3"
4: "4"

Step two

1: "1"
2: "2" <- remove
3: "3"
4: "4"

the result would be

1: "1"
2: "3"
3: "4"

Step three

1: "1"
2: "3"
3: "4" <- remove

final result

1: "1"
2: "3"

even with your templist it's the case because the index is still incrementing. In step 3 you might compare to the "3" from your templist but remove the "4" from your actual list.

score 0 · Answer 4 · answered Sep 24 '14 at 14:40

Looks like this line -

 MyListOfStrings temp= new MyListOfStrings(this);

is copying the reference of the input list rather than creating a duplicate list.

As as result, you are removing the current entry, and then skipping the next entry (which is now the current) because the new object refers to the same underlying data.

You could make a list of indexes to remove and then remove them at the end of the loop. You should remove in reverse order to prevent the same problem from occurring, of course.

score 0 · Answer 5 · answered Sep 24 '14 at 14:50

For removing objects from a list while iterating over it, it is better to use/implement an Iterator for your List.

However if you want to do it without an Iterator you could do it by reducing the i-Variable by 1 every time you remove a string from the list:

 for (int i=0;i<this.size;i++){
     if (string.equals(temp.get(i))!=true){
         this.remove(i);
         i--;
     }
 }

This is needed because you copy the i+1-Element to the i-Position which you know need to check again.

Why isn't this for loop working?

5 Answers5