remove method for arrayList doesn't work

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]

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.

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);
}

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;
    }
}