Simple Timed Cache by Wrapping HashMap

Question

Just wanted to get a bit of feedback on this simple cache. My use case is to store this data on an Android client to avoid making a high volume of network calls for lookups.

I feel like maybe I should just extend HashMap with the small amount I've added, or change up my thinking and do some type of static implementation?

public class DataCache<K, V> {
    private final long mDefaultTimeout = 15;
    private long mTimeout = 0;
    private HashMap<K, DataValue<V>> dataMap;

    DataCache() {
        dataMap = new HashMap<K, DataValue<V>>();
        mTimeout = mDefaultTimeout;
    }

    DataCache(long timeoutMinutes) {
        dataMap = new HashMap<K, DataValue<V>>();
        mTimeout = timeoutMinutes;
    }

    public void put(K key, V value) {
        dataMap.put(key, new DataValue<V>(value));
    }

    public V get(K key) {
        DataValue<V> data = dataMap.get(key);
        V result = null;
        if (data != null) {
            long diff = TimeUnit.MILLISECONDS.toMinutes(System.currentTimeMillis() - data.insertTime);
            if (diff >= mTimeout) {
                dataMap.remove(key);
                data.value = null;
            }
            result = data.value;
        }
        return result;
    }

    public void setTimeout(long minutes) {
        mTimeout = minutes;
    }

    public long getTimeout() {
        return mDefaultTimeout;
    }


    private final class DataValue<T> {
        public T value;
        public long insertTime;

        DataValue(T value) {
            this.value = value;
            insertTime = System.currentTimeMillis();
            }
        }
    }
}

Answer 1

Small code duplication

The two constructors of DataCache have duplicated code:

DataCache() {
    dataMap = new HashMap<K, DataValue<V>>();
    mTimeout = mDefaultTimeout;
}

DataCache(long timeoutMinutes) {
    dataMap = new HashMap<K, DataValue<V>>();
    mTimeout = timeoutMinutes;
}

The difference between the default constructor and the second one is the use of a default timeout value. It would be better to invoke the second constructor from the default one to avoid duplication:

DataCache() {
    this(mDefaultTimeout);
}

DataCache(long timeoutMinutes) {
    dataMap = new HashMap<K, DataValue<V>>();
    mTimeout = timeoutMinutes;
}

Logic of code

You have a DataValue class that holds the time at which it was inserted in the map. For the map itself, this should be transparent: it doesn't know what the value is doing to find out if it's outdated or not. The field long insertTime goes in that direction.

But then, you are using

long diff = TimeUnit.MILLISECONDS.toMinutes(System.currentTimeMillis() - data.insertTime);

inside the map itself. This means that, somehow the map has figured it out: it knows that the values are following the current system time to know if they're outdated or not. So this couples the map with the logic of the expiration of the value.

I would suggest creating a method isExpired(timeOut) inside the class DataValue, that will tell if a given value is expired or not based on the current timeout.

private final class DataValue<T> {
    public T value;
    private long insertTime;

    DataValue(T value) {
        this.value = value;
        insertTime = System.currentTimeMillis();
    }

    public boolean isExpired(long timeOut) {
        return TimeUnit.MILLISECONDS.toMinutes(System.currentTimeMillis() - insertTime) >= timeOut;
    }

}

Note that, in the mean time, I also made the insertTime variable private because, now, the map doesn't need to know that information. The advantage of that approach is that, someday, if you decide not to rely on System.currentTimeMillis() but on something else entirely (reading info from a file, always return true, etc.), you just need to change that single method.

In the same way, I would move the

data.value = null;

logic, that reinitializes the value to a method inside the DataValue class, and make value private.

Constants

In Java, a constant, like your mDefaultTimeout should be both static and final. Also, it should be written in uppercase. So instead, I would have

private static final long DEFAULT_TIMEOUT = 15;

Note that you could add a comment to specify the unit for which 15 refers to (Is it seconds? minutes?).

Namings

Why are you prefixing all your variables by m? This does not really follow the Java naming conventions. It would be preferable to have

private long timeout = 0;

Answer 2

I feel like maybe I should just extend HashMap with the small amount I've added, or change up my thinking and do some type of static implementation?

I don't see any compelling reason to do that. Composition is usually recommended over inheritance. It's fine as it is, stick to it, until there's a good reason to change.

The fields of DataValue can be final, so it would be good to make them so.

dataMap doesn't have to be a hash map, any map would do, so declare it as such:

private Map<K, DataValue<V>> dataMap;

The get doesn't really need the result flag variable. I suggest to rewrite without that, line this:

public V get(K key) {
    DataValue<V> data = dataMap.get(key);
    if (data == null) {
        return null;
    }
    long diff = TimeUnit.MILLISECONDS.toMinutes(System.currentTimeMillis() - data.insertTime);
    if (diff >= mTimeout) {
        dataMap.remove(key);
        data.value = null;
    }
    return data.value;
}

Thanks to the early return, two things happened:

• The indentation is reduced: flatter code is generally easier to read
• Without the lines operating on the result variable, the code is shorter

You mentioned in a comment that it's good to avoid multiple return statements. That recommendation exists for overcomplicated functions where return statements are hard to follow, for example when used in deeply nested structures, or when there are many of them. The early return statement in this example doesn't fall in those cases, and overall it improves clarity.