Simple Timed Cache by Wrapping HashMap

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 {
    private final long mDefaultTimeout = 15;
    private long mTimeout = 0;
    private HashMap> dataMap;

    DataCache() {
        dataMap = new HashMap>();
        mTimeout = mDefaultTimeout;
    }

    DataCache(long timeoutMinutes) {
        dataMap = new HashMap>();
        mTimeout = timeoutMinutes;
    }

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

    public V get(K key) {
        DataValue 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 {
        public T value;
        public long insertTime;

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

Small code duplication

The two constructors of DataCache have duplicated code:

DataCache() {
    dataMap = new HashMap>();
    mTimeout = mDefaultTimeout;
}

DataCache(long timeoutMinutes) {
    dataMap = new HashMap>();
    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>();
    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 {
    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;

StackExchange Code Review Q#123801, answer score: 4