patternjavaModerate
LinkedHashMap as LRU cache
Viewed 0 times
cachelrulinkedhashmap
Problem
I have a simple implementation for a LRU cache using
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 :)
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
With this in mind, your code became this:
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.