current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 9 down vote favorite
3

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<E> {

    @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<Object,E>(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<Object,E> fCacheMap;
    private int fCacheSize;
}
share|improve this question
2  
Instead of using a synchronized wrapper for the internal map you could synchronize all methods of LRU cache itself, as shown here: source-code.biz/snippets/java/6.htm –  Landei Jun 27 '11 at 17:25
 
There is a similar question in StackOverflow: stackoverflow.com/questions/221525/… –  palacsint Oct 31 '11 at 18:30
 
My only concern would be violation of dependency injection concept. –  JavaDeveloper Oct 12 '13 at 17:19
add comment

1 Answer

up vote 7 down vote

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<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;
    }
}
share|improve this answer
 
one improvement of above code from Victor is using lock instead of synchronized methods. Using lock will improve concurrency, e.g., get operations won't block each other. –  Eric Mar 14 '13 at 15:55
 
@Eric While non-blocking method calls are preferable, it does require that the caller handles situations where the lock isn't obtained. Furthermore, calls to the methods Victor describes are very short-lived (get, or put something on a LinkedHashMap), so I doubt it will ever be a problem with synchronized. Do you see any other advantages of using locks? –  zpon Oct 24 '13 at 5:48
 
But this is not a true LRUCache. LRU = Least recently used. I don't see that the get method updates something in the cache. It should, because the get method to the last inserted object should be the youngest object in the cache. –  x-man Dec 6 '13 at 10:29
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.