trjhtr

I want to create a singleton object of a third party caching library (ThirdPartyKVStore). But I don't want to reference this class all over the place from where I want to access the cache. To address this, I created a wrapper CacheManager which is managing the lifecycle of ThirdPartyKVStore. Does it look ok?

public class CacheManager 

 private static ThirdPartyKVStore thirdPartyCache = null;
 private static volatile CacheManager cacheManager = null;

 private CacheManager()

 

 public static CacheManager getConnection(CacheConfig cacheConfig) 
 if (cacheManager == null) 
 synchronized (CacheManager.class) 
 if (cacheManager == null) 
 try 
 thirdPartyCache = new ThirdPartyKVStore(cacheConfig); 
 cacheManager = new CacheManager();
 catch (Throwable e) 
 logger.error("Fn=getConnection Msg=Cache_Initialization_Error_Occurred.", e);
 
 
 
 
 return cacheManager;
 

 public static void closeCacheConnection()
 if(thirdPartyCache != null)
 thirdPartyCache.shutdown();
 
 

 public <T> T getCacheData(String key)
 T cachedObject = null;
 String cachedStr = null;

 Entry<String> e = thirdPartyCache.get(key);
 if(e != null)
 cachedStr = e.getPayload();
 
 if (cachedStr != null) 
 TypeToken<T> typeToken = new TypeToken<T>() ;
 cachedObject = deserializeStr2POJO(cachedStr, typeToken);
 
 return cachedObject;
 


 public <T> void setCacheData(String key, T t)
 if (thirdPartyCache != null) 
 try 
 Entry<String> e = new Entry.Builder<String>(new Gson()
 .toJson(t))
 .withVersion(DEFAULT_VERSION)
 .withApplication(DEFAULT_APP)
 .build();

 thirdPartyCache.set(key, e, Integer.MAX_VALUE); 
 catch (Exception e) 
 
 
 

It has been some time since I last wrote Java, but I have some general suggestions.

Before focusing on specific details of the code I would recommend you to spend some time on indentation. Having consistent indentation helps reduce the cognitive load when reading your code.

Second I would urge you to consider if a singleton is the appropriate solution for this problem. Using a static class makes testing both the implementation and consuming code difficult. The first being your implementation sharing state between test-cases and you therefore have to spend extra time ensuring you reset the state between each test-case. The latter is due to the consuming code hiding its dependencies. You aren't able to test a consuming class in isolation as you would implicitly also test the cache manager, which in turn also involves the third party code. Furthermore imagine that if the third party implementation relies on I/O operations your tests of the consuming code now also relies on I/O operations. Unit tests especially shouldn't be concerned with I/O such as: file operations, network, databases etc.

To remedy this I would recommend looking into creating an interface, which defines only the required methods of your cache manager. From the provided code an example of an interface could be:

interface ICacheManager 

 <T> T getCacheData(String key);

 <T> void setCacheData(String key, T value);

 void closeCacheConnection();

Then have your cache manager class implement this interface and use the inversion of control principle in your consuming code to makes its dependency on the cache manager explicit. This is done by requiring an instance of your interface in its constructor (dependency injection). Now in your test-cases you are able to create mock instances of your cache manager when testing consuming code. Using an interface also allows you to provide alternative cache manager implementation in your consuming code without changing a thing. The only requirement being all cache manager implementations adheres to your ICacheManager interface.

The next thing, which caught my eye was how the getConnection() method requires an instance of CacheConfig in its parameter list. Imagine that every time your consuming code needs access to your cache manager (by calling this method) it would also need to know about your cache configurations and have an instance of this. This breaks the separation of concerns principle. You can fix this easily by using the previously proposed solution by using the inversion of control principle.

If this isn't an option I would create a factory class, which I would ensure is always seeded with an instance of the appropriate cache configurations when starting the application. Then have the factory call the getConnection() method and return the cache manager instance. You would therefore use the factory in your consuming code. You could also seed the cache manager directly with the appropriate cache configurations, but I find that more people understand the principle of the factory pattern and it would therefore make your code easier to understand.

Note that this solution still hides the dependency of either the
factory or the cache manager in consuming code as mentioned previously
and therefore retains all the mentioned side-effects.

Next I find it enjoyable to see you checking for NULL after entering the synchronized block in the getConnection() method. Not many people remember to do this. Great job!

You should also include a check to see if the connection has already been closed in the closeCacheConnection() method. Have you checked how the third party code behaves if you try to close the connection twice? Your code should not depend on such things.

In the getConnection() method you write the name of the method in the error log message. I would recommend you to not do that. If the method is renamed and the log message isn't updated the log entries wont correspond with code. As you are already providing the exception a stacktrace accompanies the message and the name of the method would therefore already be available to the logging implementation along with line numbers etc.

Now lastly a minor pain-points of mine. In the getCacheData() and setCacheData() methods you name the variables containing the cache entry as e. This name is often associated with an exception and in the setCacheData() method this is actually the case. While this not being a syntax error I find it easier to define unique names for each variable, which is also descriptive. In this case I would recommend renaming the variables to entry as it's the contents.

A dependency injection container, like Spring or Google Guice, requires either the use of configuration files or annotations to instruct the framework where and how to do the instantiation and injection of the singleton instance. Also, inversion of control means exactly that - you lose control of the life cycle of your singleton. this can be confusing.

I find that in small programs, this is an overkill. Sometimes, you just need the good old fashioned and simple singleton and move on with your life.

So, on the subject of singleton: The classic double check mechanism that you utilized in the getConnection() method is considered broken nowadays. The reason for that is that modern processors use sophisticated branch prediction and memory cache algorithms, and in some cases execute commands out-of-order all for the sake of performance. this can result in failure of the purpose of the singleton pattern, namely, creation of more than one object.

There are two solutions for that (besides using DI framework, that is):

1) static constructor

I see you already defined the thirdPartyCache variable as static (by the way, in the classic singleton pattern, the variable is defined as an instance variable) so why not use the static constructor feature to initialize it?

 public static final CacheManager connection;
 public static 
 try 
 CacheConfig cacheConfig = ... // ? 
 thirdPartyCache = new ThirdPartyKVStore(cacheConfig);
 cacheManager = new CacheManager();
 catch (Throwable e) 
 logger.error("Fn=getConnection Msg=Cache_Initialization_Error_Occurred.", e);
 
 

Usage:

CacheManager.connection.setValue(...);

Advantages:

1.1) no need to take care of thread synchronization. You are guaranteed to have a single instance by the class loading mechanism (actually, you will have one instance per class loader, but assuming you use the JVM system class loader, this will work just fine)

1.2) you can make the variable final ensuring it is not ever modified.

in fact, you don't need a getConnection() method at all. the static variable can be declared public. How's that for simplicity.

Disdvantages:

1.1) static constructor cannot receive an argument. You will need to obtain a CacheConfig by other means. read a file from the file system?

1.2) This is an eager initialization that will take place when the class CacheManager is first accessed. Assuming the program will access that class when you need to access the singleton cache store, this seems reasonable. Also, judging from the code, the initialization creates an empty cache store, so it seems a low overhead.

1.3) static constructor cannot throw an exception. You need to catch and log it in the constructor otherwise it gets lost.

2) use enum

enum values are in fact static singleton instances of the enum class! a perfect fit!

public enum CacheManager 

 connection;
 private ThirdPartyKVStore thirdPartyCache = null;

 public void setCacheConfig(CacheConfig cacheConfig) 
 thirdPartyCache = new ThirdPartyKVStore(cacheConfig);;
 

Advantages:

2.1) all the advantages of static constructor
2.2) lazy instantiation (only when you access the instance value)

Disdvantages:

2.1) still cannot receive an argument during initialization. Although an enum can receive an argument, you have to specify it in the enum value decalration. You can use a setter like described, but then again, you can use a setter with the static constructor solution as well.

2.2) cannot have a parameterized method.