Thread safe cache implementation

Question

Please review this implementation of thread-safe chache.

public class StaticCacheImpl<T extends Entity> implements StaticCache<T> 
{
 private boolean set = false;
 private List<T> cachedObjects = Collections.synchronizedList(new ArrayList<T>());
 @Override
 public synchronized boolean isSet() {
 return set;
 }
 @Override
 public synchronized void put(List<T> cachedList) {
 if (!set) {
 cachedObjects.addAll(cachedList);
 set = true;
 }
 }
 @Override
 public synchronized List<T> get() {
 return cachedObjects;
 }
 @Override
 public synchronized boolean contains(UUID uuid) {
 if(set) {
 for (T t : cachedObjects) {
 if (ObjectUtils.equals(uuid, t.getId())) {
 return true;
 }
 }
 }
 return false;
 }
 @Override
 public synchronized T getById(UUID uuid) {
 if(set) {
 for (T t : cachedObjects) {
 if (ObjectUtils.equals(uuid, t.getUuid())) {
 return t;
 }
 }
 }
 return null;
 }
}

Answer 1

• Your cache can only be initialised once with the put method (hence your set flag): why not use a constructor instead and get rid of the flag?
• If 2 items with the same UUID are identical and you only need to store one of them, using a map would simplify your design a lot
• The map gives you an O(1) algorithm for contains and getById instead of O(n) in your code
• Since the class is now immutable, you don't need to synchronize the methods any more which should result in much better result in case of heavy concurrency (no lock contention)

Here is what I would propose:

public class StaticCacheImpl<T extends Entity> implements StaticCache<T> 
{
 //Assumes that 2 items with the same UUID only need to be stored once
 private final Map<UUID, T> cache = new HashMap<UUID, T>();
 public StaticCacheImpl(List<T> cachedList) {
 for (T t : cachedList) {
 cache.put(t.getUuid(), t);
 } 
 }
 @Override
 public boolean isSet() {
 return true;
 }
 @Override
 public void put(List<T> cachedList) {
 throw new IllegalStateException("Cache has already been set");
 }
 @Override
 public List<T> get() {
 return new ArrayList<T> (cache.values()); //defensive copy
 }
 @Override
 public boolean contains(UUID uuid) {
 return cache.containsKey(uuid);
 }
 @Override
 public T getById(UUID uuid){
 return cache.get(uuid);
 }
}

If you can't use a constructor and need the put method to work as in your original design, then the only change I would make is to use a map, but you would still need to synchronize the methods and use the set flag.

• Vlad Minaev: Thanks for your review. I really need put method for adding new values.
• Karussell: why not use ehcache?? its simple, efficient and works