HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

LinkedHashMap as LRU cache

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
cachelrulinkedhashmap

Problem

I have a simple implementation for a LRU cache using LinkedHashMap.

  • I want it to be as generic as possible.



  • This is not for production use, just practice, so I don't care


if its thoroughly robust as far as it
is correct. However, I will welcome
any comments, especially the ones
which might make this better with
simple changes :)

  • Are there any other ways of doing this?



class LRUCache {

    @SuppressWarnings("unchecked")
    LRUCache(int size)
    {
        fCacheSize = size;

        // If the cache is to be used by multiple threads,
        // the hashMap must be wrapped with code to synchronize 
        fCacheMap = Collections.synchronizedMap
        (
            //true = use access order instead of insertion order
            new LinkedHashMap(fCacheSize, .75F, true)
            {                                
                @Override
                public boolean removeEldestEntry(Map.Entry eldest)  
                {
                    //when to remove the eldest entry
                    return size() > 99 ;   //size exceeded the max allowed
                }
            }
        );
    }

    public void put(Object key, E elem)
    {
        fCacheMap.put(key, elem);
    }

    public E get(Object key)
    {
        return fCacheMap.get(key);
    }

    private Map fCacheMap;
    private int fCacheSize;
}

Solution

First, use generics properly.

Second, why do you track the cache size if when you need it, you get a hardcoded 99? I will assume that was some sort of mistake.

Third, if you really want to annoy most truely professional java developers and course instructors (i.e, not these that write Java code as if they were programming in C#, C++ or Pascal), prefix your class and variable names. If you don't want to do so, do not make this.

Fourth, I would personally prefer to handle synchronization myself. This way, if the class is expanded to include some method that makes two or more operations in the map, I will not break it. I added the atomicGetAndSet method to show this. If I relied in the synchronization provided by the Collections.synchronizedMap instead of my own, the atomicGetAndSet method would not be atomic and it would break if LRUCache instances are used concurrently.

With this in mind, your code became this:

public class LRUCache {

    private final Map cacheMap;

    public LRUCache(final int cacheSize) {

        // true = use access order instead of insertion order.
        this.cacheMap = new LinkedHashMap(cacheSize, 0.75f, true) {                                
            @Override
            protected boolean removeEldestEntry(Map.Entry eldest) {
                // When to remove the eldest entry.
                return size() > cacheSize; // Size exceeded the max allowed.
            }
        };
    }

    public synchronized void put(K key, V elem) {
        cacheMap.put(key, elem);
    }

    public synchronized V get(K key) {
        return cacheMap.get(key);
    }

    public synchronized V atomicGetAndSet(K key, V elem) {
        V result = get(key);
        put(key, elem);
        return result;
    }
}

Code Snippets

public class LRUCache<K, V> {

    private final Map<K, V> cacheMap;

    public LRUCache(final int cacheSize) {

        // true = use access order instead of insertion order.
        this.cacheMap = new LinkedHashMap<K, V>(cacheSize, 0.75f, true) {                                
            @Override
            protected boolean removeEldestEntry(Map.Entry<K, V> eldest) {
                // When to remove the eldest entry.
                return size() > cacheSize; // Size exceeded the max allowed.
            }
        };
    }

    public synchronized void put(K key, V elem) {
        cacheMap.put(key, elem);
    }

    public synchronized V get(K key) {
        return cacheMap.get(key);
    }

    public synchronized V atomicGetAndSet(K key, V elem) {
        V result = get(key);
        put(key, elem);
        return result;
    }
}

Context

StackExchange Code Review Q#3138, answer score: 10

Revisions (0)

No revisions yet.