Singleton Design Pattern Implementation: Bill Pugh I have been trying to implement the Bill Pugh Singleton Design Pattern using a private static class to return the singleton retrofit instance. So far I have been able to come up with the following code. Can someone help me review this code and confirm if this is a proper implementation of the singleton design pattern suggested by Mr. Bill Pugh himself, or suggest a proper way of implementing this?

public class SingletonRetrofit {

    private SingletonRetrofit(){}

    private static class SingletonRetrofitHelper
    {   private static OkHttpClient okHttpClient;
        private static Retrofit retrofit;

        private static Retrofit makeRetrofit()
        {
            HttpLoggingInterceptor interceptor = new HttpLoggingInterceptor();
            interceptor.setLevel(HttpLoggingInterceptor.Level.BODY);
            okHttpClient = new OkHttpClient.Builder()
                    .addInterceptor(interceptor)
                    .readTimeout(20, TimeUnit.SECONDS)
                    .connectTimeout(15, TimeUnit.SECONDS)
                    .build();

            retrofit = new Retrofit.Builder()
                    .baseUrl("http://ci.draftserver.com/hornsbyapp/webservice/")
                    .client(okHttpClient)
                    .addConverterFactory(ScalarsConverterFactory.create())
                    .addConverterFactory(GsonConverterFactory.create())
                    .build();

            return retrofit;

        }




    public static Retrofit getInstance()
    {
        return SingletonRetrofitHelper.makeRetrofit();
    }

}
I read the article Java Singleton Design Pattern Best Practices with Examples, it is an interesting discussion about Singleton Pattern and thread safe.

The basic pattern:

public class TrivialSingletonTest {
    private static TrivialSingletonTest instance;

    private TrivialSingletonTest() {}

    public static TrivialSingletonTest getInstance() {
        if (instance == null) {
            instance = new TrivialSingletonTest();
        }
        return instance;
    }

    public static void main(String[] args) {
        TrivialSingletonTest st1 = getInstance();

        TrivialSingletonTest st2 = getInstance();

        String className = "[" + TrivialSingletonTest.class.getName() + "] ";

        if (st1 == st2) {
            System.out.println(className + "Same object");
        } else {
            System.out.println(className + "Is not a singleton");
        }
    }
}


This is a basic implementation of the Singleton Pattern with a main() method that test it.

The issue with this implementation is just in a multi threading program, where you could have more than 1 instances.

So the fix proposed:

public static synchronized TrivialSingletonTest getInstance() {
    if (instance == null) {
        instance = new TrivialSingletonTest();
    }
    return instance;
}


The issue here is just about performances, that's why the proposition of a different approach, using a private static inner class as an helper to obtain the singleton instance.

The main advantages of this solution is that is thread safe without the need of synchronized, and that the instance is is created only after the first getInstance(), instead of a creation when the class is loaded (thanks to the static stuff).

Now about your code.

As I saw your code is not working.

First issue: your class is SingletonRetrofit but the object type of your instance is Retrofit. So I'm not sure you want to use all the main class as an helper or just a typo.

Second issue: you create the instance every time you call getInstance() this is not the purpose of a singleton.

Your code should look like:

public class SingletonRetrofit {

    private SingletonRetrofit(){}

    private static class SingletonRetrofitHelper
    {   
        private static final SingletonRetrofit INSTANCE = new SingletonRetrofit();
    }

    public static SingletonRetrofit getInstance()
    {
        return SingletonRetrofitHelper.INSTANCE;
    }

    private Retrofit makeRetrofit(final String apiBaseUrl)
    {
        HttpLoggingInterceptor interceptor = new HttpLoggingInterceptor();
        interceptor.setLevel(HttpLoggingInterceptor.Level.BODY);
        OkHttpClient okHttpClient = new OkHttpClient.Builder()
                    .addInterceptor(interceptor)
                    .readTimeout(20, TimeUnit.SECONDS)
                    .connectTimeout(15, TimeUnit.SECONDS)
                    .build();

        return new Retrofit.Builder()
                .baseUrl(apiBaseUrl)
                .client(okHttpClient)
                .addConverterFactory(ScalarsConverterFactory.create())
                .addConverterFactory(GsonConverterFactory.create())
                .build();
    }
}


So the makeRetrofit() will became a method of the singleton class that you can call in your code to create the Retrofit object as needed.

If you want to create this Retrofit object just once, and the singleton purpose was this, then you have to design your singleton class in a way that this class cover all the Retrofit class interface.