3

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?

5 Answers 5

6

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.

Sign up to request clarification or add additional context in comments.

2 Comments

try to write a test maybe and so you can see where it fails
@AnonymousCoder: "doesn't work" could mean any number of things. I suspect that you're passing in a Card reference which isn't in the deck, but you haven't given us enough information to know. See tinyurl.com/so-hints
3

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

3 Comments

It's not always good to override equals/hashCode. Often reference equality is absolutely fine. I suspect that in this case the OP ought to override equals, but I wouldn't claim that that's always the right thing to do.
@JonSkeet: AFAIK, only case reference equality is fine is, String literals (or) immutable objects, which is not the case in OP example.
No, it's fine any time you're looking for the exact same object, rather than just some notion of an "equal" object. Note that Card is immutable as far as I can see, assuming Suit and Rank are immutable.
2

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.

Comments

0

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.

Comments

0

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.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.