How to improve this session cache to search faster

Question

I have this code which will store user session credentials:

import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;

public class SessionCache
{

    /**
     * Default Constructor
     */
    public SessionCache()
    {
    }

    /**
     * Map which which will store Connections Objects by connection_id
     */
    private static final Map<String, ActiveConnections> cache = new LinkedHashMap<>();

    /**
     * Object which will store connection credentials
     */
    public class ActiveConnections
    {

        private int one;
        private int two;
        private int three;

        public ActiveConnections(int one, int two, int three)
        {
            this.one = one;
            this.two = two;
            this.three = three;
        }

        public int getOne()
        {
            return one;
        }

        public void setOne(int one)
        {
            this.one = one;
        }

        public int getTwo()
        {
            return two;
        }

        public void setTwo(int two)
        {
            this.two = two;
        }

        public int getThree()
        {
            return three;
        }

        public void setThree(int three)
        {
            this.three = three;
        }

    }

    /**
     * Get Connection credentials by connection_id
     *
     * @param connection_id
     * @return
     */
    public Object getCache(String connection_id)
    {
        return cache.get(connection_id);
    }

    /**
     * Insert new Connection Object into the cache
     *
     * @param connection_id
     * @param one
     * @param two
     * @param three
     */
    public void addCache(String connection_id, int one, int two, int three)
    {
        /**
         * @ You may want to check if entry already exists for connection depends on logic in your application.
         * @ Otherwise this will replace any previous entry for connection_id
         */
        if (!cache.containsKey(connection_id))
        {
            cache.put(connection_id, new ActiveConnections(one, two, three));
        }

    }

    /**
     * Remove Connection from the cache based on connection_id
     *
     * @param connection_id
     */
    public void removeCache(String connection_id)
    {
        cache.remove(connection_id);
    }

    /**
     * Get the number of all connections
     *
     * @return
     */
    public int countCacheSize()
    {
        return cache.size();
    }

    /**
     * Clear all cache
     */
    public void flushCache()
    {
        cache.clear();
    }

    public List<String> sendAllKeys()
    {
        List<String> list = new ArrayList<>(cache.keySet());
        return list;
    }

    public Object sendKeyValue(int key)
    {
        Object mapKey = cache.keySet().toArray()[key];
        return mapKey;
    }

}

Can you help me to improve this code? I would like to make a while cycle which searches for a value into the Map every 5 seconds for example expired session. Could you please help me to improve the code and make it lightweight as much as possible.

Answer 1

1. First you should rename your instance members in ActiveConnections class - having methods and variables named getOne(), getTwo(), etc, are horrible because at first you'd expect them to return the int value of one, two, three (e.g., returning 1, 2, 3) but in reality they most likely won't.
2. You should either put a lock on your cache or use a ConcurrentMap or you will sooner or later experience race condition errors.
3. Your send methods should be renamed to get because you are not sending them anywhere, you are returning them.
4. You should consider setting a memory/element limit on your cache; ex. it should only hold 101 cached items and when this limit is reached you apply a Least Recently Used policy and remove the X amount of LRU cache items.
5. Your getCache(..) and addCache(..) should be renamed because you are not getting nor adding a cache, but rather a cache item.
6. Your getCache(..) should have ActiveConnection as the return type

If you want a kind of Sliding Expiration policy where you remove items that are older than 5 seconds there are a few things to keep in mind:

• 5 seconds are a very short period of time; and
• You need a way to map a timestamp to a cache item. A very simple way of doing this is making a nested class, e.g., CacheItem, which only holds a timestamp and a value and your map will consist of a <string key, CacheItem value>

edit

Here is an example of a very simple cache class I wrote in C# (just for reference). For the test cases I've ran it through it works fine, but you might want to check into existing cache libraries.

public class SimpleCache
{
    public const int MAX_CACHE_SIZE = 101;
    public long CacheExpirationTicks { get; set; }
    private ConcurrentDictionary<string, CacheItem> _cache = new ConcurrentDictionary<string, CacheItem>();

    public SimpleCache() { CacheExpirationTicks = 30000; /*default*/ }

    public void Add(string key, object value)
    {
        //if the cache limit is reached we apply a LRU policy on the cache and remove half of the cache ( MAX_CACHE_SIZE / 2 ) least recently used
        if (_cache.Count == MAX_CACHE_SIZE)
        {
            foreach (var cacheItem in _cache
                .OrderByDescending(x => x.Value.LastAccessed)
                .Skip(MAX_CACHE_SIZE / 2))
            {
                CacheItem ignored;
                _cache.TryRemove(cacheItem.Key, out ignored);
            }

        }
        _cache.TryAdd(key, new CacheItem() { Value = value, LastAccessed = DateTime.Now.Ticks });
    }

    public object this[string key]
    {
        get
        {
            if (_cache.ContainsKey(key))
            {
                if ((_cache[key].LastAccessed + CacheExpirationTicks) > DateTime.Now.Ticks)
                {
                    return _cache[key].Value;
                }
                CacheItem ignored;
                _cache.TryRemove(key, out ignored);
            }
            return null;
        }
    }

    internal sealed class CacheItem
    {
        //despite of it's name, LastAccess will *AND SHOULD* only be set once. The cache will validate its item in the get and add function and remove them by itself
        public long LastAccessed { get; set; }
        public object Value { get; set; }
    }
}

And you would implement it like this

public class Program
{
    public static SimpleCache simpleCache = new SimpleCache();

    static void Main(string[] args)
    {
        DataTable dt = GetDataTableFromCSVFile("myCSV.csv");
    }

    private static DataTable GetDataTableFromCSVFile(string csv_file_path)
    {
        object obj;
        if ((obj = simpleCache[csv_file_path]) != null)
        {
            return (DataTable)obj;
        }
        else
        {
            var csvData = ExpensiveOperation();
            var key = csv_file_path;

            simpleCache.Add(key, csvData);

            return csvData;
        }
    }
}