Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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");
share|improve this question
2  
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. –  Konrad Rudolph Apr 6 '11 at 9:24
    
@Konrad, ah! Well that helps then. I was worried that the initialization could (in rare multi-threaded cases) happen more than once. –  jjnguy Apr 6 '11 at 12:25
9  
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. –  Athas Apr 24 '11 at 23:17
    
Here synchronization may not works correctly, all is explained in Effective Java from Joshua Bloch, or some sample in Wikipedia –  cl-r Sep 27 '12 at 6:16
1  
@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.". –  mindvirus Jan 14 '13 at 14:11

9 Answers 9

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.

share|improve this answer
    
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. –  Tom Nov 16 '13 at 20:51
    
@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). –  Konrad Rudolph Nov 18 '13 at 8:39
    
OK, I missed the private constructor being the only one. final wouldn't hurt for clarity though. –  Tom Nov 18 '13 at 12:16

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;
share|improve this answer
1  
This is not at all a singleton. –  mjcopple Apr 7 '11 at 15:32
    
Yeah, sorry. Enums are different from singletons. –  jjnguy Apr 7 '11 at 15:43
12  
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. –  romacafe Apr 7 '11 at 18:46
    
@rom, good point. I'd like to see this example expanded upon. –  jjnguy Apr 9 '11 at 2:21

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.

share|improve this answer

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

share|improve this answer
    
Yes - he must use an enum to have an absolute singleton –  Mr_and_Mrs_D Jul 21 '13 at 12:40

I would have done this:

public class Api {

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

    public static SyncApi getInstance() {
        return api;
    }
}
share|improve this answer

I did like this:

public class Api{

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

    private Api(){
    }

    public static SyncApi getInstance() {
        if(api!=null)
            return api;
    }
}
share|improve this answer

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...

share|improve this answer

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 static 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 static enum SingletonHolder {
        INSTANCE;
        private static final Api singleton = new Api();
    }

    public Api api() {
        return Api.SingletonHolder.singleton;
    }
}
share|improve this answer
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.

share|improve this answer
1  
if(null == instance) Yoda-style, this sounds like. –  Mat's Mug Jan 20 at 3:40

protected by Mat's Mug Jan 20 at 4:00

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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