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.