0

I am trying to create an LRU cache using a LinkedHashMap and the iterator. It works for many of the test cases. But for some of the large test cases, it produces a few incorrect outputs for the get().

To me, the code makes perfect sense. Perhaps, I missing something or maybe my understanding of the linkedHashMap is incomplete.

Could anyone find the bug in the implementation? Unfortunately, I cannot post the test case as it is too large. You can try to run this code on https://leetcode.com/problems/lru-cache/.

class LRUCache {
    private LinkedHashMap<Integer, Integer> map;
    private int maxCap;

    public LRUCache(int capacity) {
        this.map = new LinkedHashMap<>();
        this.maxCap = capacity;
    }

    public int get(int key) {
        if (map.containsKey(key)) {
            int val = map.get(key);
            map.remove(key);
            map.put(key, val);
            return val;
        }
        return -1;
    }

    public void put(int key, int value) {
        if (maxCap == map.size()) {
            Integer val = map.get(key);
            if (val != null) {
                map.remove(key);
            } else {
                Iterator<Integer> itr = map.keySet().iterator();
                if (itr.hasNext()) {
                    itr.next();
                    itr.remove();
                }
            }
        }
        map.put(key, value);
    }
}
user3329481
  • 1
  • 1
    The typical way to get an LRU cache is to use the access-ordered LinkedHashMap constructor and to override removeEldestEntry. – Louis Wasserman Feb 01 '20 at 01:15
  • @LouisWasserman I know just wanted to explore more possible ways to solve it. Plus, removeEldestEntry provides an abstraction that I don't like, maybe I will have a look at the source code later. – user3329481 Feb 01 '20 at 03:51

1 Answers1

0

I could be missing something but it seems like your put function is not removing the least recently used item. Instead, it seems to remove the second item from the front (you get the iterator, call next() once, and remove). Do you want something like what I have below?

`

public void put(int key, int value) {
        if (maxCap == map.size()) {
            Integer val = map.get(key);
            if (val != null) {
                map.remove(key);
            } else {
                Iterator<Integer> itr = map.keySet().iterator();
                // call next until we have the last value
                while (itr.hasNext()) {
                    // not the last value, so skip it
                    itr.next();
                }
                // remove the last value
                itr.remove()
            }
        }
        map.put(key, value);
    }`
jteezy14
  • 436
  • 4
  • 11
  • I figured it out. My solution is not changing the order of most recent element when the map is not full and the key already exists in the map. I need to use else condition for the outer if. else { Integer val = map.get(key); if (val != null) { map.remove(key); } } – user3329481 Feb 01 '20 at 04:48
  • I am using the iterator correctly, it is the linkedhashmap so the iterator is going to return the key in the insertion order. So, you are supposed to remove the first element i.e. the LRU element. – user3329481 Feb 01 '20 at 04:50