TreeSet object removal does not work even after implementing Comparator

Question

I have tried to implement AStar Algorithm in Java with Eclipse. My positions in the graph are represented by Objects. I use a TreeSet to store the positions and implemented a comparator for the object specific sorting. However at one line the code is supposed to remove the current object from the TreeSet, which does not work. I managed to use pollFirst() instead and the algorithm worked. However I could not find the reason why treeSet.remove(Object) should not work.

I added the boolean equals and compareTo. Both are true so according to equals and compareTo current is equal to openSet.first() however openSet.remove(current) is unable to remove current from openSet

I added the whole code! I tested it on codewars with huge test cases so the code works if I use pollFirst() instead of remove(current)

Edit: After reading the JavaDoc for Set Interface (https://docs.oracle.com/javase/7/docs/api/java/util/Set.html) I found the following warning:

Great care must be exercised if mutable objects are used as set elements. The behavior of a set is not specified if the value of an object is changed in a manner that affects equals comparisons while the object is an element in the set.

I suspect this has to do with my problem. However it is still strange why the program works when I replace openSet.remove(current) with openSet.pollFirst()

Edit2: I changed the Comparator according to the suggestions by Loris Securo. Unfortunately it is still not working

public class Finder implements Comparable<Finder> {

public int i; // All integers are initialized to zero.
public int j;
public int id;
public int fScore; 
public int gScore;
public ArrayList<Finder> neighbours = new ArrayList<Finder>();
public Object character;

@Override
public String toString() {
    return "Finder [i=" + i + ", j=" + j + ", gScore=" + gScore + ",fScore =" + fScore + ", id=" + id
            + ", character=" + character + "]";
}

public static void main(String[] args) {


    String a = "......\n" + "......\n" + "......\n" + "......\n" + "......\n" + "......";
    Object[] mazeParts = a.split("\n");
    System.out.println("mazeParts is " + Arrays.toString(mazeParts));
    Object[][] maze = new Object[mazeParts.length][];

    int r = 0;
    for (Object t : mazeParts) {
        System.out.println("t is " + t);
        maze[r++] = ((String) t).split("");
    }
    Finder[][] mazeOfnodes = new Finder[mazeParts.length][maze[0].length];
    int id = 0;
    for (int i = 0; i < maze.length; i++) {
        for (int j = 0; j < maze[i].length; j++) {
            System.out.println("maze" + i + j + " is " + maze[i][j]);
            Finder node = new Finder(); 
            node.character = maze[i][j];
            node.i = i;
            node.j = j;
            node.id = id;
            mazeOfnodes[i][j] = node;
            id++;
            if (i == 0 && j == 0) {
                node.fScore = heuristic(i, j, mazeOfnodes);
                node.gScore = 0;
            } else {
                node.fScore = Integer.MAX_VALUE;
                node.gScore = Integer.MAX_VALUE;
            }
        }
    }
    for (int i = 0; i < mazeOfnodes.length; i++) { 
        for (int j = 0; j < maze[i].length; j++) {
            mazeOfnodes[i][j].neighbours = mazeOfnodes[i][j].findNeighbours(i, j, mazeOfnodes);
            System.out.println("mazeOfnodes is " + mazeOfnodes[i][j]);
        }
    }


}

public static int findWay(Finder[][] myMaze) {
    Finder goal = myMaze[myMaze.length - 1][myMaze.length - 1];
    TreeSet<Finder> openSet = new TreeSet<Finder>();
    openSet.add(myMaze[0][0]);
    TreeSet<Finder> closedSet = new TreeSet<Finder>();
    while (openSet.size() != 0) {
        Finder current = openSet.first();
        if (current == goal) {
            return current.gScore;
        }
        boolean equals = current.equals(openSet.first());
        boolean compareTo = (current.compareTo(openSet.first()) == 0);
        openSet.remove(current); //if I use openSet.pollFirst() the code   works fine. I tested it on Codewars with several huge test cases
        boolean success = openSet.remove(current);
        System.out.println("success is " + success);

        closedSet.add(current);
        for (Finder s : current.neighbours) {
            if (closedSet.contains(s)) {
                continue;
            }
            int tentative_gScore = current.gScore + 1;
            if (tentative_gScore >= s.gScore) {
                continue;
            }
            s.gScore = tentative_gScore;
            s.fScore = s.gScore + heuristic(s.i, s.j, myMaze);
            if (!openSet.contains(s) && !s.character.equals("W")) {
                openSet.add(s);
            }
        }
    }
    return -1;
}

private ArrayList<Finder> findNeighbours(int i, int j, Finder[][] maze) {
    if (i > 0) {
        neighbours.add(maze[i - 1][j]);
    }
    if (i < maze.length - 1) {
        neighbours.add(maze[i + 1][j]);
    }
    if (j < maze.length - 1) {
        neighbours.add(maze[i][j + 1]);
    }
    if (j > 0) {
        neighbours.add(maze[i][j - 1]);
    }
    return neighbours;
}

public static int heuristic(int i, int j, Object[][] mazeFinal) {
    int distance = (int) Math.sqrt(Math.pow(mazeFinal.length - 1 - i, 2) + Math.pow(mazeFinal.length - 1 - j, 2));
    return distance;


    public int compareTo(Finder2 arg0) {
    if (this.fScore < arg0.fScore) {
        return -1;
    } else if (this.id == arg0.id) { // If id is the same, fScore is the same too
        return 0;
    } else if (this.fScore == arg0.fScore) { //If id is different, fScore could still be the same
        return 0;
    } else {
        return 1;
    }
}

Answer 1

Your compareTo method doesn't work properly on a TreeSet because it doesn't satisfy the rule that if A > B then B < A.

In your case, if fScore are equals (and id are different) then A > B, but also B > A.

A TreeSet is backed by a self balancing binary tree that is built based on the compareTo method.

Because of that inconsistency the tree might be unable to find a specific object, since it wouldn't be able to correctly choose to perform a search on the left branch or on the right branch.

For example, if you have 3 objects A, B and C with the same fScore:

1. you add A, now it is the root

2. you then add B, the compareTo says that it is greater than A, so it could generate this tree (now the root is B):

     B
    /
   A
   

3. you then add C, the compareTo says that it is greater than B, so it could generate this tree:

     B
    / \
   A   C
   

4. now you want to remove A, which is present, but, using the comparator, A is greater than B, so the tree checks the right branch, there it finds C and it stops, so the method is unable to find and delete A

pollFirst instead could simply pick the leftmost object, so it is always able to remove something, but the removed object is going to depend on the implementation.

This problem was also discussed in this blog post: Algosome - Java: The misouse of a TreeSet which is based on this forum thread: Java Programming Forum - TreeSet contains(obj) doe not behave as expected.