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]

There must be an easy/better way of doing what you want to do — Pablo Fernandez
– Pablo Fernandez, Commented Nov 21, 2010 at 14:00
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? ;-) — T.J. Crowder
– T.J. Crowder, Commented Nov 21, 2010 at 14:07

T.J. Crowder · Accepted Answer · 2013-07-26 15:40:28Z

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.

Community · Accepted Answer · 2017-05-23 12:29:03Z

4

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

edited May 23, 2017 at 12:29

CommunityBot

11 silver badge

answered Nov 21, 2010 at 14:01

Eric

98.1k54 gold badges257 silver badges389 bronze badges

1 Comment

Eric Over a year ago

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

richj · Accepted Answer · 2010-11-21 15:12:08Z

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

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

Collectives™ on Stack Overflow

remove method for arrayList doesn't work

3 Answers 3

Comments

1 Comment

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

1 Comment

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related