What is the issue with this java singleton class implementation?

Question

I have come across another article in stackexchange on various ways to implement java singleton. One of the ways shown is the following example. It has been voted very low. Wanted to understand why. What is an efficient way to implement a singleton pattern in Java?

public class Singleton {

    private static Singleton instance = null;

    static {
          instance = new Singleton();
          // do some of your instantiation stuff here
    }

    private Singleton() {
          if(instance!=null) {
                  throw new ErrorYouWant("Singleton double-instantiation, should never happen!");
          }
    }

    public static getSingleton() {
          return instance;
    }

}

My guess is that the code itself is right, but the explanation next to it was misleading. — millimoose, Jan 18 '13 at 00:58
Also the return type of `getSingleton()` is missing so the code doesn't even compile. — millimoose, Jan 18 '13 at 01:00
I don't think that null-check in your constructor is required because the constructor is private so it's not accessible for (re-)invocation outside the class. — Bhesh Gurung, Jan 18 '13 at 01:07
@millimoose: But reflection can bypass that null-check too, right? — Bhesh Gurung, Jan 18 '13 at 01:18

score 5 · Answer 1 · edited May 23 '17 at 12:18

5

As @Craig says in the comments:

Not true. static variables are initialized along with static blocks when the class is loaded. No need to split the declaration.

Essentially it was down voted because it was misinformation, a lot of what he was saying was just plain not true. Specifically, initializing a static variable with a static method will occur when the class is loaded, while the author claimed that this was not the case.

His argument also doesn't really make sense, "data insertion" could just be done within the constructor.

With that said, the above code will work fine, it's just an odd way of doing it, and arguably the least stylistic.

edited May 23 '17 at 12:18

Community

1
1

answered Jan 18 '13 at 00:58

Alex DiCarlo

4,851
18
34

2

Looking at it again, not only was it misinformation, but it actually makes no sense whatsoever, he's literally moving all of the work that could have been done in the constructor into a static block. So that's two strikes against his post... I'm surprised that he got an up vote for it. – Alex DiCarlo Jan 18 '13 at 01:11

sasankad · Answer 2 · 2013-01-18T01:11:49.167

following solution make sure it's thread safe

public class Singleton {
    // Private constructor prevents instantiation from other classes
    private Singleton() { }

    /**
    * SingletonHolder is loaded on the first execution of Singleton.getInstance() 
    * or the first access to SingletonHolder.INSTANCE, not before.
    */
    private static class SingletonHolder { 
            public static final Singleton INSTANCE = new Singleton();
    }

    public static Singleton getInstance() {
            return SingletonHolder.INSTANCE;
    }

}

score 0 · Answer 3 · answered Jan 18 '13 at 01:05

0

This is not a good way to implement it.

As static variables are initialized at JVM load time, just make the singleton final:

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

    private Singleton()
    {
        // build it
    }

    public static Singleton getInstance()
    {
        return INSTANCE;
    }
}

answered Jan 18 '13 at 01:05

fge

119,121
33
254
329

i too, prefer this one, but if initializing is expensive and not always called, the other variant with synchronized makes sense,too – AlexWien Jan 18 '13 at 01:07
bye the way: this variant is called "Singleton with Eager instantiantion", the other is lazy instantiation – AlexWien Jan 18 '13 at 01:10
1

The other factor to consider is contention. Once you make Singleton thread safe, you have a synchronized getInstance method which means that if 100 clients are trying to get it, they all have to wait. One imagines that it wouldn't slow things down that much, but the static initialization removes that problem. – Rob Jan 18 '13 at 01:11
Yes, but I am really not a fan of lazy instantiation at all. I have just never used it... I use Guava's LoadingCache when I have to build expensive stuff. – fge Jan 18 '13 at 01:11
Another thing, its a perfect singleton, which makes it evil in unit test, the final keyword prohibits resetting the singleton, this even does not work via Reflection. – AlexWien Jan 18 '13 at 01:14

What is the issue with this java singleton class implementation?

3 Answers3