Singleton Factory - Implementation using Java 8

Question

I want to implement a generic singleton factory pattern where I pass the Class of the required object as a parameter and the factory class should check in the map if there's already an object created for it, if its, return the object from map. If not, create a new instance, put it in the map and return the instance.

I can have a generic return type as Object, but I don't want to cast the returned object at every place I call the get instance method.

The below is the code: I get a compilation error at the line c.cast(instance);

We do not use spring/dependency injection, but trying implement common class to take care of creating all singleton objects.

public class SingletonFactory {
    public static Map<String,Object> objectFactory = new HashMap<String, Object>();

    public static <T extends Object> T getInstance(Class<?> c){
        String key = c.toString();
        Object instance= objectFactory.get(key);
        if (instance == null) {
            synchronized (c) {
                try {
                    instance = c.newInstance();
                    objectFactory.put(key, instance);
                } catch(IllegalAccessException | InstantiationException e) {
                    throw new RuntimeException("Exception while creating singleton instance for class : "+key+" - Exception Message : "+e);
                }
            }
        }
        return c.cast(instance);
    }
}

you are expecting to return generic from the function, but instance is an object which is not generic — mkjh, Jan 04 '19 at 02:15
Yeah, so just trying to figure out how to declare my map to have a generic type. Is it at least possible to implement a factory like this. I think I am close, but don't know how to write that code. — Sanjeev, Jan 04 '19 at 02:17
you should change `Class>` to Class extends T> so when you do a casting, it casts instance to T — mkjh, Jan 04 '19 at 02:23

retodaredevil · Accepted Answer · 2020-05-10T17:15:44.837

First, I can point out that <T extends Object> can be replaced with just <T> because everything in Java, involving generics, must be an Object.

The second part that you're really close on is Class<?> c. That says that you can pass any class in and it will return whatever type T is. c.cast(instance) can be replaced with (T) instance if you think that looks better but, there's actually a difference which goes into more detail here: Java Class.cast() vs. cast operator .

The final code looks like this:

public class SingletonFactory {
    public static Map<String,Object> objectFactory = new HashMap<String, Object>();

    public static <T> T getInstance(Class<T> c){
        synchronized (c) {
            String key = c.toString();
            Object instance= objectFactory.get(key);
            if (instance == null) {
                try {
                    instance = c.newInstance();
                    objectFactory.put(key, instance);
                } catch(IllegalAccessException | InstantiationException e){
                    throw new RuntimeException("Exception while creating singleton instance for class : "+key+" - Exception Message : "+e);
                }
            }
            return c.cast(instance);
            // or
            return (T) instance;
        }
    }
}

Also if you really wanted to, you could keep everything in your original code and cast instance to T at the end of the method and it should work. The only thing is your method calls would look like SingletonFactory.getInstance<Foo>(Foo.class) instead of SingletonFactory.getInstance(Foo.class). That is because of the Class<?> in your original code instead of Class<T>.

EDIT: I also changed the code to synchronize earlier thanks @Khan9797

Since you are using Lazy initialization I would suggest you to use Double Checked Locking. There is a possibility for two threads checking instance value for null. If the instance variable is null they both try to create. So you would end up overwritten hashmap for given key. — Khan9797, May 10 '20 at 15:08
@Khan9797 you're right. I edited my answer to synchronize earlier. Can I synchronize on the class object, or should I just synchronize the method itself? Or does it matter? If I keep it the way it is, by synchronizing on the class, would I have to also have some lock when editing the HashMap as well? — retodaredevil, May 10 '20 at 17:18

score 4 · Answer 2 · answered Jan 04 '19 at 02:21

4

Firstly, you need to synchronize much earlier, you should simply synchronize the method, otherwise, you can create extra instance in a race condition.

Secondly, you should define the generic of the method like this:

public static <T> T getInstance(Class<? extends T> c)

answered Jan 04 '19 at 02:21

Jai

8,165
2
21
52

I don't have to move the synchronize up to the method level - i want to synchronize the class only when an instance for that class is not found. I don't think there's a need to lock the entire method. – Sanjeev Jan 04 '19 at 19:06
Two thread can look at if(instance == null) and assume instance is null and create two objects – Cybermonk Dec 10 '19 at 07:52
Why do you need `Class extends T> c` ? In HashMap they all stored as `Object`, not `T` – Khan9797 May 10 '20 at 15:35

Khan9797 · Answer 3 · 2020-05-10T16:22:37.777

First of all, getInstance() is not thread-safe in terms of creating the new instance. There is a chance that you could create multiple instances of a given class when multiple threads running simultaneously and variable == null is true.

public class SingletonFactory {
    private static Map<Class, Object> objectHolder = new HashMap<>();

    public <T> T getInstance(Class<T> clazz) {
        Object instance = objectHolder.get(clazz);
        if(instance == null) {
            synchronized (clazz) {
                if(instance == null) {
                    try{
                        instance = clazz.newInstance();
                        objectHolder.put(clazz, instance);
                    } catch (Exception e) {
                        // do some logging and maybe exit the program. Since the it would affect how whole system works.
                    }                            
                }
            }
        }

        return clazz.cast(instance);
    }  
}

But the better approach would be using eager initialization instead of lazy initialization. The reason why we need to synchronize the critical section is that we are creating those instances when we need it. So it became a readers-writers problem. But if we only do reading process then we don't need to synchronize since we are not going to modify its value. If you know all the classes which are going to be created and need to be accessed we could just initialize them in the first place. So that we would get rid off the synchronized performance drawback

public class SingletonFactory {
    private static Map<Class, Object> objectHolder = new HashMap<>();

    private Map<Class, Object> initialize() {
        Map<Class, Object> objectHolder = new HashMap<>();
        // create some objects and put it into Map
        return objectHolder;        

    }           

    public <T> T getInstance(Class<T> clazz) {
        Object instance = objectHolder.get(clazz);            
        return clazz.cast(instance);
    }  
}

Singleton Factory - Implementation using Java 8

3 Answers3