0
\$\begingroup\$

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();
    }

}
\$\endgroup\$
  • \$\begingroup\$ I could be wrong, but it looks to me that every time you call getInstance() you are going to create a new Retrofit instance. And this is not a Singleton I suppose. Instead you should create the instance just the first time. Could you add in your answer a link on the site where you find this approach? \$\endgroup\$ – Mario Santini Dec 29 '16 at 10:46
  • \$\begingroup\$ i tried checking the singleton object to be null and made an instance only if it is null. This works perfectly and i have implemented it in my projects. Then i found this method. journaldev.com/1377/… Have a read and share insights. \$\endgroup\$ – rakshak manandhar Dec 29 '16 at 11:44
1
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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