non duplicates in a Set

[Rob McBryde]

Hi All,

I am in the process of writing a small program where I have created a very simple class called Vertex that has a String as a field. I am now attempting to add a number of instances of my Vertex class into a HashSet. However, my resultant HashSet has duplicates in it.

I have added a equals method and compareTo method in my Vertex class to attempt to allow my HashSet to uniquely identify Vertexes but I am clearly missing something. Can anyone point me in the right direction. Do I need to override the HashCode in my Vertex class?

public class Vertex implements Comparable<Vertex>{
	
        // name of the Vertex
	private final String name;

       // constructor
	public Vertex(String name)
	{
		this.name = name;
	}
	
	public String getName()
	{
		return this.name;
	}
	
	@Override
	public String toString()
	{
		return this.name;
	}

	public int compareTo(Vertex o) {
		return (this.name.compareTo(o.name));		
	}
	
	public boolean equals(Object o)
	{
		if(this.name.equalsIgnoreCase(((Vertex) o).name))
			return true;
		else 
			return false;
	}

}

public class Settest {

	public static void main(String[] args)
	{
		Set<Vertex> test = new HashSet<Vertex>();
		
		Vertex v1 = new Vertex("A");
		Vertex v2 = new Vertex("B");
		Vertex v3 = new Vertex("A");
		Vertex v4 = new Vertex("C");
		
		test.add(v1);
		test.add(v2);
		test.add(v3);
		test.add(v4);
	
	}
}

This results in a Set of [A, B, A, C] when I would like to have no duplicates, hence my choice in using a Set.

Thanks in advance

[Matthew Brown]

Rob McBryde wrote:Do I need to override the HashCode in my Vertex class?

Yes, that's it. You should always override hashCode() whenever you override equals(), as they should be consistent with each other (see the contract descriptions in the Javadocs for Object). HashSet relies on hashCode() working correctly (the "Hash" in the name is not a coincidence!).

[Rob Spoor]

And your equals method has a huge bug. If you pass null or an instance of an incompatible class the method should return false. Your implementation will throw a NullPointerException / ClassCastException.

[Stephan van Hulst]

Not only is it bugged, it uses a poor construct:

if (something)
  return true
else
  return false;

[Rob McBryde]

Thanks for the advice, I have re-written my equals method and overridden the hashCode() method within my Vector class. Have tested this and no duplicate Vectors are being added to my HashSet. Hurray!

@Override
    public boolean equals(Object o)
    {
       if (o == null)
           return false;
       if(!this.getClass().equals(o.getClass()))
           return false;
       if(this.name.equalsIgnoreCase(((Vertex)o).name))
           return true;
       else
           return false;
    }

    @Override
    public int hashCode()
    {
        int hash = 1;
        return hash + this.name.hashCode();
    }

I appreciate the comments, a little unsure about Stephan's comment about the poor construct of my initial equals method. Would you be able to further advise why it is poor and what I should be doing to improve it ?

Thanks

[Paul Beckett]

I think what Stephan is getting at is that you can replace:

if(something){
   return true;
}else{
   return false;
}

with:

return something;

thus improving the readability of your code.

[Rob McBryde]

Thanks Paul,

Makes perfect sense to me now. I've updated my code and will I ensure I stick to the more readable and less verbose approach from now on.