41
\$\begingroup\$

I was wondering if my code will produce a true singleton. I am creating an Android app, and all activities should access my API through one instance of the SyncApi class.

public class Api {

    private static SyncApi api = null;

    static {
        synchronized (api) {
            if (api == null) {
                api = new ApiFActory().getSyncApi();
            }
        }
    }

    public static SyncApi getInstance() {
        return api;
    }
}

Use of the class would look like:

SyncApi api = Api.getInstance();
api.logOn("jjnguy", "pa55w0rd");
\$\endgroup\$
7
  • 3
    \$\begingroup\$ If you don’t want laziness, why not initialise the SyncApi instance directly in the declaration? No need for the static constructor. Java guarantees that initialisation of static members will only happen once. \$\endgroup\$ Commented Apr 6, 2011 at 9:24
  • 9
    \$\begingroup\$ I'd just like to point out that Singletons are considered an anti-pattern mostly now due to the difficulty in testing. Also you haven't made the constructor private so this isn't actually a Singleton. \$\endgroup\$
    – Athas
    Commented Apr 24, 2011 at 23:17
  • 1
    \$\begingroup\$ @Athas Singletons are considered an anti-pattern because it takes this nice concept of encapsulation and controlled flow of data and says "hey, let's go back to having global variables now.". \$\endgroup\$
    – mindvirus
    Commented Jan 14, 2013 at 14:11
  • 1
    \$\begingroup\$ Why why why you init a static variable to null ? It will be null - will be even if not static - bad idiom \$\endgroup\$ Commented Jul 21, 2013 at 12:22
  • 1
    \$\begingroup\$ synchronized (null) is mutch more effective way to create a NPE and prevent the loading of the class. BTW: The NPE will be absolutly thread-save. \$\endgroup\$
    – Grim
    Commented Mar 3, 2014 at 14:12

7 Answers 7

43
\$\begingroup\$

In Java there’s an established idiom for creating a thread-safe singleton, due to Bill Pugh:

public class Api {
    private Api() { }

    private static class SingletonHolder { 
        public static final SyncApi INSTANCE = new ApiFactory().getSyncApi();
    }

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

This implementation is both lazy and completely thread-safe, without the need to explicit synchronization and checking. This has the advantage that it eliminates the possibility of subtle race conditions and redundant locking.

\$\endgroup\$
4
  • \$\begingroup\$ How is the SyncApi class defined? I believe this implementation can be broken if Api and/or SyncApi are not final. One could extend these classes and create objects in ways completely uncontrolled by this code. Every instance of SyncApi's subclass would obviously be an instance of SyncApi as well. The enum-based solution is not affected by this because all enums are implicitly final. \$\endgroup\$ Commented Nov 16, 2013 at 20:51
  • \$\begingroup\$ @Tom Actually I just copied & pasted OP’s code to some extent, I was actually meaning the two classes to be one (i.e. SyncApi = Api), then this code is safe (Api cannot be extended). \$\endgroup\$ Commented Nov 18, 2013 at 8:39
  • \$\begingroup\$ OK, I missed the private constructor being the only one. final wouldn't hurt for clarity though. \$\endgroup\$ Commented Nov 18, 2013 at 12:16
  • 1
    \$\begingroup\$ Don’t use this code — use the enum-based safe singleton idiom given in the answer by user aivikiss. \$\endgroup\$ Commented Jul 31, 2019 at 12:28
37
\$\begingroup\$

The best and simple method was gived by Joshua Bloch in 《Effective Java 2th edition》

public enum Api {
    INSTANCE;
}

you can use it like this

Api api = Api.INSTANCE;
\$\endgroup\$
4
  • 1
    \$\begingroup\$ This is not at all a singleton. \$\endgroup\$
    – mjcopple
    Commented Apr 7, 2011 at 15:32
  • \$\begingroup\$ Yeah, sorry. Enums are different from singletons. \$\endgroup\$
    – jjnguy
    Commented Apr 7, 2011 at 15:43
  • 22
    \$\begingroup\$ Why? Enums are classes, are they not? If you define additional non-static methods on the Api class above, it will behave exactly like a singleton, because it IS a singleton. \$\endgroup\$
    – romacafe
    Commented Apr 7, 2011 at 18:46
  • \$\begingroup\$ @rom, good point. I'd like to see this example expanded upon. \$\endgroup\$
    – jjnguy
    Commented Apr 9, 2011 at 2:21
5
\$\begingroup\$

If you want thread-safe and lazy, I completely agree with the answer provided by Konrad Rudolph.

If you want just thread-safe without laziness (which is also very common), then you can initialize INSTANCE in the upper class, not from inner one.

Here is a good article with many tests and examples:
http://www.javaworld.com/javaworld/jw-04-2003/jw-0425-designpatterns.html

But both approaches don't give you an absolute singleton, since different ClassLoaders will create different instances. It might look like a nearly impossible case, but in my experience I faced a bug which was cause by such a situation.

Here is an article about absolute singletons:
http://surguy.net/articles/communication-across-classloaders.xml

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Yes - he must use an enum to have an absolute singleton \$\endgroup\$ Commented Jul 21, 2013 at 12:40
4
\$\begingroup\$

Your setting your api to null did not help you spot your NullPointerException

synchronized (api) // api == null, you should use Api.class

As to the singleton please, please, please use an enum. It is the recommended pattern since java 5. Now, if you want lazy loading :

public class Api {

    private Api() {}

    private enum SingletonHolder {
        INSTANCE;

        private static final Api singleton = new Api();

        Api getSingleton() {
            return singleton;
        }
    }

    public Api api() {
        Api.SingletonHolder.INSTANCE.getSingleton();
    }
}

Call :

 api().logOn("jjnguy", "pa55w0rd");

You may dispose of getSingleton() and access singleton directly inside Api

public class Api {

    private Api() {/*your costly init here*/}

    private enum SingletonHolder {
        INSTANCE;
        private static final Api singleton = new Api();
    }

    public Api api() {
        return Api.SingletonHolder.singleton;
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ you do not need to use static and final keywords in enum class declaration as enum classes are already provide static and final. \$\endgroup\$
    – Enes Unal
    Commented Oct 1, 2014 at 12:46
  • \$\begingroup\$ @EnesUnal: correct (see also stackoverflow.com/a/253282/281545), edited \$\endgroup\$ Commented Oct 1, 2014 at 13:31
3
\$\begingroup\$

Well, this is definitely not a Singleton, because this code is legal: Api.api=new Api();

You should at least define api variable as final and at least private constructor to avoid instantiation.

See @Konrad Rudolph's for proposed solution.

\$\endgroup\$
0
2
\$\begingroup\$

I agree with all the other answers but your problem, as you state it, is not to have a single API instance but to have a single SyncApi instance.

You will have no way to prevent a user from doing:

SyncApi api = new ApiFActory().getSyncApi();

You have to make SyncApi a Singleton or encapsulate it's usage in your Api class and not give access to it outside your Api...

\$\endgroup\$
0
\$\begingroup\$
public class Singleton {
      private static Singleton instance = null;

      public static Singleton getInstance(){
         if(null == instance){
          synchronized(Singleton.class){
             //double check
               if(null == instance){
                instance = new Singleton();
              }
           }  
         }
         return instance;
       }

}

This can be used to ensure only single instance is created. Or just use Enums.

\$\endgroup\$
1
  • 4
    \$\begingroup\$ if(null == instance) Yoda-style, this sounds like. \$\endgroup\$ Commented Jan 20, 2014 at 3:40

Not the answer you're looking for? Browse other questions tagged or ask your own question.