HashMap adding all items even when duplicate

I have been trying without any luck to make a list of all points in a model. When i execute this

    HashList<Point> points=new HashList<Point>(16);

    //add +y side
    points.add(new Point(-5.0,5.0,-5.0));
    points.add(new Point(-5.0,5.0,5.0));
    points.add(new Point(5.0,5.0,5.0));

    points.add(new Point(-5.0,5.0,-5.0));
    points.add(new Point(5.0,5.0,5.0));
    points.add(new Point(5.0,5.0,-5.0));

    //add -x side
    points.add(new Point(-5.0,5.0,-5.0));
    points.add(new Point(-5.0,-5.0,-5.0));
    points.add(new Point(-5.0,-5.0,5.0));

    points.add(new Point(-5.0,5.0,-5.0));
    points.add(new Point(-5.0,-5.0,5.0));
    points.add(new Point(-5.0,5.0,5.0));

    int length=points.length(); //equals 12, 6 expected

    Point a=new Point(-5.0,5.0,-5.0);
    Point b=new Point(-5.0,5.0,-5.0);
    int aHashCode=a.hashCode(); //-737148544
    int bHashCode=b.hashCode(); //-737148544
    boolean equals=a.equals(b); //true

points containts 12 points which is the number I started with. I want all duplicates found which should result in only 6 points in table.

if (map.containsKey(e)) {

in HashList for some reason never gets executed. Any ideas?

HashList:

package dataTypes;

import java.util.ArrayList;
import java.util.HashMap;

public class HashList<E> {
    private HashMap<E,Integer> map;
    private ArrayList<E> data;
    private int count=0;

    public HashList() {
        map=new HashMap<E,Integer>();
        data=new ArrayList<E>();
    }

    public HashList(int size) {
        map=new HashMap<E,Integer>(size);
        data=new ArrayList<E>(size);
    }

    public int add(E e) { //returns key
        if (map.containsKey(e)) {
            return map.get(e);
        } else {
            map.put(e, count);
            data.add(count,e);
            return count++;
        }
    }

    public int getKey(E e) {
        return map.get(e);
    }

    public E get(int key) {
        return data.get(key);
    }

    public int length() {
        return count;
    }
}

Point:

package geometry3D;

/**
 * 3D point location or vector
 * 
 * @author Matthew Cornelisse
 * @version 2014-09-02-004500
 */
public class Point
{
    // instance variables - replace the example below with your own
    public double x;
    public double y;
    public double z;

    /**
     * Constructor for objects of class Point
     */
    public Point()
    {
        // initialise instance variables
        x = 0;
        y = 0;
        z = 0;
    }

    public Point(double x, double y, double z)
    {
        this.x=x;
        this.y=y;
        this.z=z;
    }
    public Point(Point a) {
        x=a.x;
        y=a.y;
        z=a.z;
    }

    /**
     * Normailizes the point to have distance from center of 1
     *  
     */
    public void normalize()
    {
        // put your code here
        double length=Math.sqrt(x*x+y*y+z*z);
        x/=length;
        y/=length;
        z/=length;
    }



    //implements Shape
    public void rotateX(double angle){
        double newY=Math.cos(angle)*y-Math.sin(angle)*z;
        double newZ=Math.sin(angle)*y+Math.cos(angle)*z;
        y=newY;
        z=newZ;
    }
    public void rotateY(double angle){
        double newX=Math.cos(angle)*x-Math.sin(angle)*z;
        double newZ=Math.sin(angle)*x+Math.cos(angle)*z;
        x=newX;
        z=newZ;
    }
    public void rotateZ(double angle){
        double newX=Math.cos(angle)*x-Math.sin(angle)*y;
        double newY=Math.sin(angle)*x+Math.cos(angle)*y;
        x=newX;
        y=newY;
    }
    public void rotate(Vector axis, double angle){
        //source:  http://inside.mines.edu/fs_home/gmurray/ArbitraryAxisRotation/
        double oldX=x;
        double oldY=y;
        double oldZ=z;
        double sinA=Math.sin(angle);
        double cosA=Math.cos(angle);
        Point offset=axis.offset();
        Point vector=axis.vector();
        double u=vector.x;
        double v=vector.y;
        double w=vector.z;
        double a=offset.x;
        double b=offset.y;
        double c=offset.z;


        x=(a*(v*v+w*w)-u*(b*v+c*w-u*oldX-v*oldY-w*oldZ))*(1-cosA)+oldX*cosA+(-c*v+b*w-w*oldY+v*oldZ)*sinA;
        y=(b*(u*u+w*w)-v*(a*u+c*w-u*oldX-v*oldY-w*oldZ))*(1-cosA)+oldY*cosA+(c*u-a*w+w*oldX-u*oldZ)*sinA;
        z=(c*(u*u+v*v)-w*(a*u+b*v-u*oldX-v*oldY-w*oldZ))*(1-cosA)+oldZ*cosA+(-b*u+a*v-v*oldX+u*oldY)*sinA;



    }
    public void move(double x, double y, double z){
        this.x+=x;
        this.y+=y;
        this.z+=z;
    }
    public void move(Vector direction,double magnitude){
        this.x+=(direction.vector().x*magnitude);
        this.y+=(direction.vector().y*magnitude);
        this.z+=(direction.vector().z*magnitude);
    }

    public boolean equals(Point compare) {
        if (Math.abs(compare.x-x)>5*Math.ulp(compare.x)) return false;
        if (Math.abs(compare.y-y)>5*Math.ulp(compare.y)) return false;
        if (Math.abs(compare.z-z)>5*Math.ulp(compare.z)) return false;
        return true;
    }
    public boolean equals(Point compare, double error) {
        if (Math.abs(compare.x-x)>error) return false;
        if (Math.abs(compare.y-y)>error) return false;
        if (Math.abs(compare.z-z)>error) return false;
        return true;
    }

    public int hashCode(){
        Double a=(Double)x;
        Double b=(Double)y;
        Double c=(Double)z;
        return a.hashCode()^Integer.rotateRight(b.hashCode(),12)^Integer.rotateRight(c.hashCode(),24);
    }

    public boolean equals(Object compare) {
        try {
            Point temp=(Point)compare;
            if (temp.x!=x) return false;
            if (temp.y!=y) return false;
            if (temp.z!=z) return false;
            return true;
        } finally {
            return false;
        }       
    }
}
Jon Skeet
people
quotationmark

The problem is your finally block in equals(Object). It's always returning false, even if you're returning true from the try block.

You're getting confused because of this:

boolean equals=a.equals(b); //true

... but that's not calling the same equals method - it's calling equals(Point).

Your equals(Object) method should be written as:

@Override public boolean equals(Object compare) {
    if (compare == this) {
        return true;
    }
    if (compare == null || compare.getClass() != getClass()) {
        return false;
    }
    Point temp = (Point) compare; // Guaranteed to work now
    return temp.x == x && temp.y == y && temp.z == z;
}

Note that:

  • As noted elsewhere, you can't come up with an equality/hash which handles tolerance... if you do any significant arithmetic with these points, you're unlikely to have exact equality any more
  • Your class has public fields, which is pretty much always a bad idea
  • Your class is mutable, which is a bad idea from the point of view of code using the hash code - collections are much easier to use correctly when the element type is immutable
  • If you make your class final, you can use instanceof instead of the getClass() check - and again, that would help prevent someone introducing a mutable subclass. (Equality relationships across an inheritance hierarchy are generally painful anyway.)

people

See more on this question at Stackoverflow