Evicting an entry from a size-limited HashMap<T, Integer>

Question

I have a HashMap with a limited size of 1000 elements. When I want to put one more, the T with the lowest related Integer in the map should be replaced with the new one.

I already have this working solution for my problem, but it seems a bit too much code for me so maybe you have a better idea :)

HashMap<T, Integer> set = new HashMap<>();
   public boolean add(T element) {
    if (set.containsKey(element)) {
        return false;
    } else if (set.size() == length) {
        Integer searchedInteger = set.values().stream().sorted().findFirst().get();
        T searchedElement = set.entrySet()
                .stream()
                .filter(x -> x.getValue().equals(searchedInteger))
                .findFirst()
                .get()
                .getKey();
        set.remove(searchedElement);
    }
    set.put(element, 0);
    return true;
}

Answer 1

Improvement of the code

Your current code can be improved concerning the logic that is removing the key having the lowest value. You are doing 2 Stream pipelines: the first one to retrieve the smallest value and the second one to get the first key mapping to that value.

You can actually do this with a single Stream pipeline. For that, we can sort the Map according to a comparator comparing the values, keep the first entry and remove the key of that entry. There is a built-in comparator for that: Map.Entry.comparingByValue(). The following does just that:

set.entrySet()
   .stream()
   .sorted(Map.Entry.comparingByValue())
   .findFirst()
   .map(Map.Entry::getKey)
   .ifPresent(set::remove);

Note that instead of calling get() "blindly" on the Optional returned by findFirst(), this code maps it to an Optional containing only the key and, if it is present, removes that key from the map.

While in this case we know that the map won't be empty (and so that get() will never throw a NoSuchElementException), it is still preferable to not rely on that and write code that is as much generic as possible.

If / else and return statements

This is the sketch of your method:

if (set.containsKey(element)) {
    return false;
} else if (set.size() == length) {
    // do something
}
// ...
return true;

I feel that adding explicitely the else hides the fact that the method has a clear separate path when the map doesn't contain the element. I would remove it and have instead:

if (set.containsKey(element)) {
    return false;
}
if (set.size() == length) {
    // do something
}
// ...
return true;

The difference here is that it becomes quite clear that the first part of the method is dealing with the case where the map doesn't have the element, and so that the rest of method can safely assume that it is not the case. It shows that the method returns early in that case.

Worth mentioning but the fact that you're returning false when an element that is already contained in the map is given as argument is really great because this is what Collection.add also mandates (if the collection does not permit duplicates) and so it adheres with the principle of least surprise.

Namings

You have named your map set. This is confusing: it implies that we're dealing with a Set instead of a Map. I would suggest renaming that variable to something that conveys more its intent (that would depend on what you do with that map, so I can't really propose any name).

Also,

HashMap<T, Integer> set = new HashMap<>();

should be replaced with

Map<T, Integer> set = new HashMap<>();

It is preferable to always code against interfaces instead of implementation.