LRU Cache in Java

Question

Problem description:

Design and implement a data structure for Least Recently Used (LRU) cache. It should support the following operations: get and put.

get(key) - Get the value (will always be positive) of the key if the key exists in the cache, otherwise return -1. put(key, value) - Set or insert the value if the key is not already present. When the cache reached its capacity, it should invalidate the least recently used item before inserting a new item.

Follow up: Could you do both operations in O(1) time complexity?

I passed 17/18 test cases with this and failed the last one due to this exceeding time constraints. I'm guessing something here isn't O(1)? I've spent hours but can't identify it.

class LRUCache {
    Map<Integer, Integer> cache;
    Queue<Integer> q;
    int capacity;

    public LRUCache(int capacity) {
        cache = new HashMap<>();
        q = new LinkedList<Integer>();
        this.capacity = capacity;
    }

    public int get(int key) {
        if (cache.get(key) == null || cache.get(key) == -1) return -1;
        int value = cache.get(key);
        q.remove(key);
        q.add(key);
        System.out.println("get() - key: " + key + " value: " + value);
        return value;
    }

    public void put(int key, int value) {
        if (cache.get(key) == null || cache.get(key) == -1) {
            if (q.size() >= capacity) {
                evict();
            }
        } else {
            q.remove(key);
        }
        q.add(key);
        cache.put(key, value);
        System.out.println("put()...key: " + key + " queue size: " + q.size());
    }

    private void evict() {
        int toRemove = q.remove();
        cache.put(toRemove, -1);
        System.out.println("Evict: " + toRemove + " queue size: " + q.size());
    }
}

/**
 * Your LRUCache object will be instantiated and called as such:
 * LRUCache obj = new LRUCache(capacity);
 * int param_1 = obj.get(key);
 * obj.put(key,value);
 */

Answer 1

class LRUCache {
    Map<Integer, Integer> cache;
    Queue<Integer> q;
    int capacity;

Is there any reason for not making these fields private?

---

    public int get(int key) {
        if (cache.get(key) == null || cache.get(key) == -1) return -1;

Why the special case if cache.get(key) == -1? I don't see that in the spec.

        int value = cache.get(key);

This method has now called cache.get(key) three times. I think this could be optimised somewhat...

        q.remove(key);

This is not O(1). I suspect that you will need to implement your own linked list to get O(1) removal from the middle.

        System.out.println("get() - key: " + key + " value: " + value);

I would recommend removing debug printing before asking for code review.

---

    public void put(int key, int value) {

My observations are similar to those on get.

---

    private void evict() {
        int toRemove = q.remove();
        cache.put(toRemove, -1);

Huh? This answers my earlier question about special cases, but raises a more fundamental question. Do you understand what the point of an LRU cache is? You should never have cache.size() > capacity.