Java object Equality

Question

The two cards c1 and c4 seem to be equal...but they are not why. I want them to be equal so that only one of them is allowed in the Set. :|

import java.util.*;
class Card2
{
 private int value;
 private String type;

 public Card2(int v,String t)
 {
  value=v;
  type=t;
 }

 public int getValue()
 {
  return value;
 }

 public String getType()
 {
  return type;
 }

 public String toString()
 {
  return(type+" "+value);
 }

 public boolean equals(Object oo)
 {
  if(!(oo instanceof Card))
  return false;

  Card cc=(Card) oo;

  if(this.getValue()==cc.getValue() && this.getType().equals(cc.getType()))
  return true;
  else
  return false;
 }

 public int hashCode()
 {
  return value;
 }

 public static void main(String args[])
 {
  HashSet<Card> deck=new HashSet<Card>();

  Card c1=new Card(5,"Spade");

  Card c2=new Card(10,"Hearts");

  Card c3=c2; //Equal Ref card entity

  Card c4=new Card(5,"Spade");

  System.out.println(c1.equals(c4));

  deck.add(c1);
  deck.add(c2);
  deck.add(c4);
  deck.add(c3);

  for(Card cc:deck)
  System.out.println(cc);
 }
}

Answer 1

Your hashCode() is inconsistent with equals().

java.util.HashSet uses hashCode. You should implement hashCode() to take the type into account.

Answer 2

They are equal (once you fix the typo by replacing Card2 by Card), your program output is:

true
Hearts 10
Spade 5

What else did you expect?

Answer 3

First of all: you called your class Card2 but refer to it as Card everywhere (including the equals() method. This answer assumes you replace all instances of Card2 with Card).

You defined equals() in a way to return true if the value and the type of the card to be the same.

c1 has a value of 5 and a type of Spade.

c4 has a value of 5 and a type of Spade.

The look pretty much the same to me.

Answer 4

What is the problem? Output on my system is:

true
Spade 5
Hearts 10

Which seems to be exactly what you want.

Answer 5

Your equals() method is wrong, try this instead:

public boolean equals(Object oo)
{
  if(!(oo instanceof Card2))
    return false;

  Card2 cc=(Card2) oo;

  return this.getValue()==cc.getValue() && this.getType().equals(cc.getType());
}

That is, you should be consistent in your usage of Card and Card2.

Also note that I changed your:

if(this.getValue()==cc.getValue() && this.getType().equals(cc.getType()))
  return true;
else
  return false;

to

return this.getValue()==cc.getValue() && this.getType().equals(cc.getType());

As this is shorter, and avoids violating a checkstyle rule. The reason why this is a good idea is that it complicates the code since what you are saying is "if something is true return true, otherwise if it is false, return false". Rather than force the reader of the code work out what you are doing, you can simply say "return something" and the eventual user of your code can more quickly understand what you are doing.

Answer 6

There is another equality check missing from your equals() method. Check whether the objects themselves are the same reference. It's a short circuit for when you compare things like "c2.equals(c3)".

public boolean equals(Object oo)
{
  if(!(oo instanceof Card2))
    return false;

  if(this == oo) //comparing an object to itself is equal
    return true;

  Card2 cc=(Card2) oo;

  return this.getValue()==cc.getValue() && this.getType().equals(cc.getType());
}

Answer 7

Your equals method needs some improvements. You should test for null and for sameness. Also, using instanceof may result in equals becoming non-commutative, if you allow subclassing and implement equals in both classes. For example if Card2 extends Card, and Card has an equals that tests for instanceof Card, and Card2 overrides this with an equals that tests for instanceof Card2, then for an instance cc2 of class Card2 and another instance cc of Card the use of instanceof means that cc.equals(cc2) is true, but cc2.equals(cc) is false, which may lead to undesirable behavior.

You could do the following:

public boolean equals(Object other) {
    // null is not equal
    if (null == other)
        return false;
    // same is equal
    if (this == other)
        return true;
    // different class, not equal
    if (other.getClass() != getClass())
        return false;

Alternatively, if you want to allow subclassing, you should test only for properties that are in the superclass. So if Card has value, and Card2 extends Card and has value and type, then your equals method should only look for instanceof Card and compare only Card's attributes:

    //if (other.getClass() != getClass())
    //    return false;
    if (!(other instanceof Card))
        return false;
    card = (Card) other;
    if (this.getValue() == card.getValue())
        return true;
    return false;
} 

Then again, all this might be completely unrelated to your problem.

Answer 8

Here is our well received Card class from a recent project; hopefully it will help you out. http://pastebin.com/qW41nwRE
The "state" is used to determine if the card is in the deck, in a hand, etc. The value is from 1 to 14 with 11 - 14 being the face cards (Jack, Queen, King, and Ace).