remove method for arrayList doesn't work

Question

Hi I have written this code that with output you can get that .remove() method doesn't work. a, b, c, and d are some Points Objects that have x and y members.

Here are a and b and c and d values, which in the if statement must be deleted for upper but it doesn't.

X :59 Y: 143
X :165 Y: 140
X :59 Y: 143
X :165 Y: 140
 System.out.println(upper.toString());
 for(int i =0;i<upper.size();i++)
 if(upper.get(i)==a||upper.get(i)==b||upper.get(i)==c||upper.get(i)==d){
 upper.remove(i);
 }
 for(int i =0;i<lower.size();i++)
 if(lower.get(i)==a||lower.get(i)==b||lower.get(i)==c||lower.get(i)==d){
 upper.remove(i);
 }
 System.out.println(upper.toString());
 System.out.println(lower.toString());
 first println : [X :108 Y: 89, X :165 Y: 140]
 second println: [X :108 Y: 89, X :165 Y: 140]
 third println : [X :105 Y: 191]

• Pablo Fernandez: There must be an easy/better way of doing what you want to do
• T.J. Crowder: Wow. Can I just say. Braces. Specifically, curly ones. Really. Or do you actively want maintenance programmers to trip over when they see this code? ;-)

Answer 1

If I'm reading your question right, you're assuming that == will compare the properties of two objects. It doesn't, that's what equals does. == tells you whether two references are to the same object instance, not to equivalent ones.

So for example:

public class Foo {
 public Foo(int x, int y) {
 this.x = x;
 this.y = y;
 }
 @override
 public boolean equals(Object other) {
 Foo otherFoo;
 if (other == null || !(other instanceof Foo)) { // or you might be more restrictive
 return false;
 }
 otherFoo = (Foo)other);
 return otherFoo.x == this.x && otherFoo.y == this.y;
 }
 @override
 public int hashCode() {
 // ...appropriate implementation of hashCode...
 }
}
Foo a = new Foo(0, 0);
Foo b = new Foo(0, 0);
System.out.println(a == b); // "false"
System.out.println(a.equals(b)); // "true"

Separately: Consider what happens when you have two consequtive matching objects in the ArrayList that you have to remove. Say they're at indexes 8 and 9 in the list. So when i == 8, you remove the item at index 8, and the one that used to be at 9 is now at 8. But then you increment i in the for loop and continue with the new item at index 9, leaving the second one untouched. If you want to modify the list while you're looping through it, consider looping backward to avoid that, or using an Iterator.

Answer 2

Two problems here. Firstly, you're removing objects from a list while you iterate it. That's not a good idea.

Secondly, I think you're misunderstanding the == operator in Java, as mentioned by @T.J. Crowder.

This is a better way of doing what you're trying to do (after you've fixed the equals issue):

List<Point> mypoints = new ArrayList();
mypoints.add(a);
mypoints.add(b);
mypoints.add(c);
mypoints.add(d);
List<Point> otherPoints = new ArrayList();
for(Point p: upper)
 for(Point myPoint: mypoints)
 {
 if(p.equals(myPoint))
 break;
 otherPoints.add(p);
 }
upper = otherPoints;

Another implementation (which only works if upper is a Set, as it will not catch duplicates):

List<Point> mypoints = new ArrayList();
mypoints.add(a);
mypoints.add(b);
mypoints.add(c);
mypoints.add(d);
for(Point myPoint: mypoints)
{
 upper.remove(myPoint);
}

• Eric: @user472221: Have you added the equals() method to you Point class as described in @T.J. Crowder's answer?

Answer 3

As Eric implies, the length of the list changes as items are removed from it, and so do the indices of all of the values after the element that has just been removed.

I'm not sure what the significance of "lower" is. I did notice that the loop that iterates through "lower" attempts to remove elements from "upper". Is this intentional?

This is my solution based on a "remove" list of points that should be removed from "upper". It is also possible to use the style of your original test except that each == check has to to be replaced by an equals() check.

If the equals(...) implementation is removed from the Point class, nothing will be removed from "upper" because the test case deliberately uses clones of the original a,b,c and d values.

import java.util.ArrayList;
import java.util.List;
import junit.framework.Assert;
import org.junit.Test;
public class TestArrayList
{
 @Test
 public void testRemove()
 {
 // Test fixture:
 Point a = new Point(115, 70);
 Point b = new Point(139, 66);
 Point c = new Point(195, 111);
 Point d = new Point(144, 165);
 List<Point> upper = new ArrayList<Point>();
 upper.add(a.clone());
 upper.add(b.clone());
 upper.add(c.clone());
 upper.add(d.clone());
 List<Point> remove = new ArrayList<Point>();
 remove.add(a.clone());
 remove.add(b.clone());
 remove.add(c.clone());
 remove.add(d.clone());
 // Assertions:
 Assert.assertTrue(upper.size() == 4);
 Assert.assertTrue(remove.size() == 4);
 // Modified code:
 System.out.println(upper.toString());
 System.out.println(remove.toString());
 for (Point p : remove)
 {
 upper.remove(p);
 }
 System.out.println(upper.toString());
 System.out.println(remove.toString());
 // Assertions:
 Assert.assertTrue(upper.isEmpty());
 Assert.assertTrue(remove.size() == 4);
 }
}
class Point implements Cloneable
{
 public int x;
 public int y;
 public Point(int x, int y)
 {
 this.x = x;
 this.y = y;
 }
 @Override
 public Point clone()
 {
 return new Point(x, y);
 }
 @Override
 public boolean equals(Object o)
 {
 if (this == o)
 {
 return true;
 }
 else if (o instanceof Point)
 {
 Point p = (Point) o;
 return x == p.x && y == p.y;
 }
 else
 {
 return false;
 }
 }
 @Override public String toString()
 {
 return "X: " + x + " Y: " + y;
 }
}

• Eric: D'oh! I was looking at the Java 1.4.2 ArrayList documentation!