Using my own object as key in a TreeMap

Question

As TreeMap sorting is based only on the keys, I'm using a custom object as key in a treemap. I have respected in my opinion the contract between equals and compareTo in this case, if the two objects are equal, te compareTo returns 0.

Below the code of the object:

public final class UserHighScore implements Comparable<UserHighScore>{

private final int userId;
private final int value;


public UserHighScore(int userId, int value) {
    this.userId = userId;
    this.value = value;
}

public int getUserId() {
    return userId;
}

public int getValue() {
    return value;
}


@Override
public boolean equals(Object obj) {
    if (obj == this) return true;
    if (!(obj instanceof UserHighScore)) {
        return false;
    }
    UserHighScore userHighScore = (UserHighScore) obj;
    return userHighScore.userId==userId;
}


@Override
public int compareTo(UserHighScore uh) {
    if(uh.getUserId()==this.getUserId()) return 0;
    if(uh.getValue()>this.getValue()) return 1;
    return -1;
}

}

And below the method that is causing the problem:

If the user id's are the same I want to return 0 to avoid duplication, so then if I do map.put(userHighscore) it should automatically replace is there is another object in the map with the same userId. However if the users are diferent, I want them to be sorted according the their values. This approach is working perfectly fine for one thread, however my app is concurrent and when there are more than a threas, it is adding duplicates to the map. My problem is with the highscores map, which is a concurrentHasmap and inside it contains a treemap.

DO you see anything wrong with my approach?

Answer 1

Updated answer

Looking better at the source of TreeMap hashCode is not a real problem.

The problem is here

if (highScores.get(levelId)==null) {
    highScores.put(levelId,Collections.synchronizedSortedMap(new TreeMap<UserHighScore,Integer>()));
}

This code is not thread safe also if highScores is a ConcurrentHashMap.

Here a possible scenario

Thread 1                                    Thread 2
----------------------------------------------------------------------
highScores.get(levelId) is null
                                            highScores.get(levelId) is null
highScores.put(levelId, ...);
                                            highScores.put(levelId, ...);

From here the two threads use a different instance of SynchronizedSortedMap.

---

Previous answer

TreeMap is not a synchronized version of Map.

If you work in a multithreading environment you need to synchronize the access to the TreeMap.

TreeMap<UserHighScore> myTree = ...
...
UserHighScore userHighScore = ...
...
synchronized(myTree) {
    // Synchronize any access to myTree
    myTree.add(userHighScore);
}

But you need also to redefine the hashCode method because you are using a Map:

Returns a hash code value for the object. This method is supported for the benefit of hash tables such as those provided by HashMap.

Remember to redefine the hashCode following the contract:

• Whenever it is invoked on the same object more than once during an execution of a Java application, the hashCode method must consistently return the same integer, provided no information used in equals comparisons on the object is modified. This integer need not remain consistent from one execution of an application to another execution of the same application.
• If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.
• It is not required that if two objects are unequal according to the equals(java.lang.Object) method, then calling the hashCode method on each of the two objects must produce distinct integer results. However, the programmer should be aware that producing distinct integer results for unequal objects may improve the performance of hash tables.