Singleton Example

[john sal]

Following is the problem description:

Class should be singleton for the same index, as object properties may vary from index to index but will remain same for the same index.

Could somebody please let me know if below code is fine or there is any better way to address this ?

getSingletonInst is used to get the singleton object and map contains index to object mapping, I am doing map lookup to check if object is already present for the given index before creating it. fileLoc property will remain same for similar indexes. I am trying to use double check locking to avoid synchronization in case of multiple threads.

public class SingletonExample {
    static volatile Map<String, SingletonExample> map = new HashMap<String, SingletonExample>();
    String fileLoc;
    
    SingletonExample(String fileLoc){
    	this.fileLoc = fileLoc;
    }
	
    public static SingletonExample getSingletonInst(String index, String fileLocation){
      SingletonExample singleEx = map.get(index);
      
      if(null == singleEx){
    	synchronized(SingletonExample.class){
           if((singleEx = map.get(index)) == null){
        	   singleEx = new SingletonExample(fileLocation);
        	   map.put(index, singleEx);
           }
    	 }
      }
      return singleEx;
    }
 }

[Anurag Verma]

Not sure if volatile is really needed here, your code looks good still would suggest small improvements -

public class SingletonExample {
    private static Map<String, SingletonExample> map = new HashMap<String, SingletonExample>();
    private String fileLoc;
     
    private SingletonExample(String fileLoc){
        this.fileLoc = fileLoc;
    }
     
    public static SingletonExample getSingletonInst(String index, String fileLocation){
      if(map.containsKey(index)){
        synchronized(SingletonExample.class){
           if(!map.containsKey(index)){
               map.put(index, new SingletonExample(fileLocation));
           }
         }
      }
      return map.get(index);
    }
 }

[Rob Spoor]

Java 8 to the rescue:

public class SingletonExample {
    private static final Map<String, SingletonExample> MAP = new ConcurrentHashMap<>();

    private String fileLoc;
    
    private SingletonExample(String fileLoc) {
    	this.fileLoc = fileLoc;
    }

    public static SingletonExample getSingletonInst(String index, String fileLocation) {
        return MAP.computeIfAbsent(index, k -> new SingletonExample(fileLocation));
    }
}

First of all this makes everything final, and makes the Map final. Otherwise code in the same package could overwrite anything, and even create instances.

Next I'm using Map's computeIfAbsent function. This will return the value for the key if it's present, or otherwise use the function to create a new value, insert it into the Map, and then return that value. And because I'm using a ConcurrentHashMap that one action is guaranteed to be atomic, so no custom synchronization needed. And ConcurrentHashMap often even locks just part of the Map, not the whole thing, making it even more efficient.

[salvin francis]

Here's a one that I had read in a stack overflow example a while back...

class MyLazyLoadedSingleton{
	private static class SingletonLoader{
		private final static MyLazyLoadedSingleton INSTANCE = new MyLazyLoadedSingleton(); 
	}
	public static MyLazyLoadedSingleton getInstance(){
		return SingletonLoader.INSTANCE;
	}
	private MyLazyLoadedSingleton() {
		System.out.println("I am loaded");
	}
}
public class Tester {
	public static void main(String[] args) {
		System.out.println("Lazy Loading...");
		MyLazyLoadedSingleton instance = MyLazyLoadedSingleton.getInstance();
		System.out.println("Done");
	}
}

output:

Lazy Loading...
I am loaded
Done

Edit:
Here's a link mentioning it :
Initialization-on-demand_holder_idiom#Example_Java_Implementation

[Dave Tolls]

I've never seen a case where the lazy loading of the singleton instance makes a difference.
Every singleton I've seen (and I haven't seen any for quite some time now, it has to be said) has only ever had one accessible static method (getInstance), so I don't see the point in it.

[Raymond Tong]

Lazy initialisation would be good if the initialisation process may be expensive and not necessary.
There is also another way of creating Singleton -> Enum

[Stephan van Hulst]

Raymond Tong wrote:Lazy initialisation would be good if the initialisation process may be expensive and not necessary.

Which in my book, shouldn't be. If initialization is expensive for whatever reason, you're doing the wrong things in the wrong methods.

For instance, setting up a connection to a database shouldn't be done in the constructor of the connection object. Connections should provide an open() method that can be called *after* the object has been created. This allows clients to prepare objects without too much overhead, and use them when they're ready for expensive operations.

[Dave Tolls]

Raymond Tong wrote:Lazy initialisation would be good if the initialisation process may be expensive and not necessary.
There is also another way of creating Singleton -> Enum

Irrelevant.
If you need the instance using getInstance and there are no other static methods on the singleton (and I've never seen one with anything other than getInstance) then why lazy initialise?

public class SomeSimgleton {
    private static final SomeSingleton INSTANCE = new SomeSingleton();

    public SomeSingleton getInstance() {
        return INSTANCE;
    }
}

INSTANCE will only be initialised the first time getInstance is called. That's how class loading works.
There is no point tucking that attribute away in a holder class.

[Raymond Tong]

http://martinfowler.com/bliki/LazyInitialization.html

Remember that this is an optimization technique to help responsiveness in situations where a client doesn't need the lazy initialized value. As with any optimization you shouldn't use this technique unless you have a real performance problem to solve.

Lazy initialization may not always be the right choice but it may be used if it is needed.

public class SomeSimgleton {
    private static final SomeSingleton INSTANCE = new SomeSingleton();
 
    public SomeSingleton getInstance() {
        return INSTANCE;
    }
}

For your code above, how the caller to get an instance if getInstance is not a static method ? and it should have a private constructor?
I am not saying one approach is better than the others, use the right tool to do the right job.

[Junilu Lacar]

Stephan van Hulst wrote:

Raymond Tong wrote:Lazy initialisation would be good if the initialisation process may be expensive and not necessary.

Which in my book, shouldn't be. If initialization is expensive for whatever reason, you're doing the wrong things in the wrong methods.

I would qualify that some more. Some things are truly expensive to initialize, regardless where you do it and inspite of doing it the proper way. The question is how much does that operation impact other things like application response time or concurrency (see the wikipedia entry on double-check locking). Maybe it's more of a "wrong place, wrong time" thing rather than "wrong things in the wrong methods".

[Winston Gutkowski]

Raymond Tong wrote:Lazy initialization may not always be the right choice but it may be used if it is needed...

I'm afraid you're on a non-starter with most of us here. My argument is simply that if you need something like 'initialization on demand', it's a symptom that your code (or your approach) already has problems elsewhere.

However, back to john's original question: I don't see this as a singleton AT ALL. It's a HashMap (or a wrapper to one), pure and simple. If you absolutely feel that there must only be ONE of them, then make it an enum; but I'd say even that may be overkill.

What if you want different versions of this class for your test, development and production environments? Seems perfectly possible to me, since the class is dealing with "file location"s, but you've just shot yourself in the foot by making it a singleton.

My 2¢.

Winston

[Dave Tolls]

Raymond Tong wrote:For your code above, how the caller to get an instance if getInstance is not a static method ? and it should have a private constructor?
I am not saying one approach is better than the others, use the right tool to do the right job.

Typo...

And serves me right for typing it straight into the forum rather than via an IDE!

That should be a static method.
(Edit: and yes, a private constructor)

However, unless your Singleton has some other accessible static thing then there is (in Java) absolutely no point in making it lazy initialised, as the first time that class is accessed (and hence the first time it is loaded) is when someone calls getInstance...at which point you need an instance anyway.

If someone has a reasonable example of a Singleton that has more than a getInstance() non-private static method (or attribute) then yes, that would be a possible use for it, but as I said earlier I have never seen one where the other parts would be accessed before the getInstance.

[Rob Spoor]

Raymond Tong wrote:For your code above, how the caller to get an instance if getInstance is not a static method ? and it should have a private constructor?
I am not saying one approach is better than the others, use the right tool to do the right job.

I present to you Log4J's NullAppender.getInstance(). They fixed it with another method (getNullAppender()) after I (and probably some other people) pointed out the very same error

[salvin francis]

I think we all are going a bit tangential to what the OP had actually asked (Yes, I am guilty too )

john sal wrote:Class should be singleton for the same index, as object properties may vary from index to index but will remain same for the same index.

Winston Gutkowski has already pointed out that it is not a singleton at all.

I believe that the getInstance() method should never accept an argument. This is because the singleton should be stateless.

By very definition, a "single"ton implies a single object which you are clearly not trying to achieve. You need a separate object for every key.

A word or caution is that even if you create a singleton in the one of the ways suggested by any of the previous posts, there is a small problem. You are creating (directly or lazily) an object with global scope. Its still frowned upon by a lot of critics who say this is an anti-pattern. Look at one of the reasons below:

Winston Gutkowski wrote:What if you want different versions of this class for your test, development and production environments? Seems perfectly possible to me, since the class is dealing with "file location"s, but you've just shot yourself in the foot by making it a singleton.

[Paul Clapham]

Rob Spoor wrote:Next I'm using Map's computeIfAbsent function. This will return the value for the key if it's present, or otherwise use the function to create a new value, insert it into the Map, and then return that value. And because I'm using a ConcurrentHashMap that one action is guaranteed to be atomic, so no custom synchronization needed. And ConcurrentHashMap often even locks just part of the Map, not the whole thing, making it even more efficient.

I struggled to produce something like that prior to Java 8 (in a concurrent environment) and ended up with something which, um, didn't fail. But I'm totally going to rewrite using that method!

[Winston Gutkowski]

Rob Spoor wrote:Next I'm using Map's computeIfAbsent function.

<sigh>
All this version 8 stuff I'm still discovering by osmosis.
</sigh>

Long overdue. My only criticism of a lot of these new methods (putIfAbsent() being another) is that I really dislike the name. Why not just call it compute()? And for putIfAbsent() - ensure().

A name should explain what a method does, not how it does it or why I'm calling it.

However, have a cow for pointing it out to me.

Winston

[Rob Spoor]

Winston Gutkowski wrote:

Rob Spoor wrote:Next I'm using Map's computeIfAbsent function.

Long overdue. My only criticism of a lot of these new methods (putIfAbsent() being another) is that I really dislike the name. Why not just call it compute()?

Because that also exists: Map.compute. But they could have renamed that one of course.

And for putIfAbsent() - ensure().

A name should explain what a method does, not how it does it or why I'm calling it.

I agree with you, but it's what we've been given...

However, have a cow for pointing it out to me.

Winston

Thanks

[Winston Gutkowski]

Rob Spoor wrote:Because that also exists: Map.compute. But they could have renamed that one of course.

Erm...yeah. reMap() perhaps?

I'm clearly going to have to get to grips with these new "expanded" APIs. In some cases I like them, but in many others I think they're "overcooked" and badly named.

All you need to do is look at Comparator to see a world gone mad AFAIC - 16 new methods, when there was only 1 before? That's not to say that I don't agree with some of them (and had already designed around 3 of them myself), but this "rush to functionality" bothers me a lot.

I doubt very much if Sun would have done it.

Winston