CODEWITHSUNDEEP
java
Anonymous Coder
Anonymous Coder

Reputation: 51

Can't remove object from ArrayList

I'm working on a card game. I can't figure out how to remove a Card from an ArrayList no matter what. This is the code I'm using:

private List<Card> cardDeck = new ArrayList<Card>();

public void removeCard(Card card) {
    for (Iterator<Card> it = cardDeck.iterator(); it.hasNext();) {
        Card nextCard = it.next();
        if (nextCard.equals(card)) {
            cardDeck.remove(card);
            System.out.println("removed " + card);
        }
    }
}

And here's the card class, incase you need it:

    public class Card {

    public Card(Rank rank, Suit suit) {
        this.rank = rank;
        this.suit = suit;
    }

    public Rank getRank() {
        return rank;
    }

    public Suit getSuit() {
        return suit;
    }

    @Override
    public String toString() {
        return getRank().toString().toLowerCase() + " of "
                + getSuit().toString().toLowerCase();
    }

    private Rank rank;

    private Suit suit;

}

I've tried everything but it just won't remove. Any tips?

Upvotes: 3

Views: 6733

Answers (5)

Andreas Schmid
Andreas Schmid

Reputation: 1

Comment public methods before you write them. Demonstrate your intention with the comment. It would help me right now.

As someone previously mentioned you should use the iterator to delete the card.

The equals method is not well used. Because you haven't overriden the equal() method in the Card class, therefore the equals() method of the Object class will be called. This equals method only returns true, if the Card instances are the same! This is a very special case of the equals() specification. Is that your intention? If yes it would be clearer to write:

if (nextCard == otherCard){..

equal normally means:

Indicates whether some other object is "equal to" this one.

The equals method implements an equivalence relation on non-null object references:

It is reflexive: for any non-null reference value x, x.equals(x) should return true.
It is symmetric: for any non-null reference values x and y, x.equals(y) should return true if and only if y.equals(x) returns

true. It is transitive: for any non-null reference values x, y, and z, if x.equals(y) returns true and y.equals(z) returns true, then x.equals(z) should return true. It is consistent: for any non-null reference values x and y, multiple invocations of x.equals(y) consistently return true or consistently return false, provided no information used in equals comparisons on the objects is modified. For any non-null reference value x, x.equals(null) should return false.

Source: http://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#equals%28java.lang.Object%29

How did you come to the conclusion that the remove method doesn't work?

You use the remove method of the ArrayList(), which you shouldn't use, if you previously use an Iterator. But the remove method of the ArrayList() has an advantage: it returns a boolean value. Always check return values! Because in your case the card must always be removed, I think, and the method is public you should throw an exception, if the return value is false. (Could the carddeck be empty before you call remove()?)

An other way to check the remove method, is to check if the ArrayList's size has decreased.

Upvotes: 0

Marko Topolnik
Marko Topolnik

Reputation: 200148

Your Card class should really be an enum, which would force a one-to-one relationship between each distinct card and each distinct Java object. Then you wouldn't need to implement equals and hashCode and could in fact use == instead of equals. You could use your Card constants in enums, employ the very efficient EnumSet and much more. Do yourself a favor and make enum Card.

Upvotes: 2

Jon Skeet
Jon Skeet

Reputation: 1500145

When you're iterating over a collection, the only way you're meant to remove an item is to call remove on the iterator. So you should use:

if (nextCard.equals(card)) {
    it.remove();
    System.out.println("removed " + card);
}

Note that as you haven't overridden equals, this is really just a reference comparison, so you'll only go into the body of the if statement if nextCard and card are references to the exact same object.

Of course if you just want the method to remove the card, you should be able to just change it to:

public void removeCard(Card card) {
    cardDeck.remove(card);
}

... with the same caveat around equality, of course.

To override equals (and hashCode for consistency) I would first make Card a final class, then write:

public final class Card {
    ...

    @Override
    public boolean equals(Object other) {
        if (!(other instanceof Card)) {
            return false;
        }
        Card otherCard = (Card) other;
        return this.rank == otherCard.rank &&
               this.suit == otherCard.suit;
    }

    @Override
    public int hashCode() {
        int hash = 17;
        hash = hash * 31 + rank.hashCode();
        hash = hash * 31 + suit.hashCode();
        return hash;
    }
}

That's assuming that Rank and Suit are enums (for the reference equality check in equals to be appropriate). You probably want to add nullity checks in the Card constructor, too.

Upvotes: 6

kosa
kosa

Reputation: 66637

When objects are used in collections, it is always good to override equals() and hashcode(). Otherwise equality condition may fail while doing lookup.

Another approach to resolve your issue would be:

use it.remove() instead of cardDeck.remove(card);

Example:

for (Iterator<Card> it = cardDeck.iterator(); it.hasNext();) {
        Card nextCard = it.next();
        if (nextCard.equals(card)) {
            it.remove();
            System.out.println("removed " + card);
        }
    }

Upvotes: 3

Qnan
Qnan

Reputation: 3744

First, it's not a good idea to remove something while iterating over the container. Second, you have to implement the equals() method. If you also implement the hashCode() method, you'll be able to use the ArrayList's built-in remove() method instead of writing one of your own.

Upvotes: 0

Related Questions