Correct way to implement a Singleton field

Question

I have a helper class that does HTTP operations on Android. Every method uses a client field. I think it's a good idea to make this client a singleton since every method in the class uses it.

The problem is that I don't know 100% what's the best way to implement this singleton since it's only a field in my helper class.

public class HttpHelper {
    private static OkHttpClient client;

    private HttpHelper() { }

    private static OkHttpClient getClient() {
        if(client == null) {
            client = new OkHttpClient();
            client.setConnectTimeout(TIMEOUT, TimeUnit.SECONDS);
        }
       return client;
    }

    // method that uses client:
    public static String getHTML(String cookie, String URL) throws IOException {

        Request request = new Request.Builder().url(URL).header("Cookie", cookie).build();
        Response response = getClient().newCall(request).execute(); // using client
        if (!response.isSuccessful()) throw new IOException("Unexpected code " + response);

        return readStream(response.body().byteStream());
    }
}

I am using methods like getHTML() (yes, the class has more) all over my project.

1. Is it OK that I use a lot of static methods like getHTML()?

2. Should I be implementing the HttpHelper class as singleton and use non static methods like HttpHelper.getInstance().getHTML() (where the client is created in the getInstance() method of the HttpHelper?

Answer 1

IMHO, it's ok to use static methods if the object (HTTPHelper in your case) doesn't store data and can't have multiple states.

A little suggestion:

    private static final OkHttpClient client;
    static {
        client = new OkHttpClient();
        client.setConnectTimeout(TIMEOUT, TimeUnit.SECONDS);
    }

if you initailize your client in this way, you won't need getClient method at all!

Actually, what concerns me in your code is whether it must be used by multiple threads or not. What will happen of one thread call getHTML while client is used in another thread pending from answer?

Addition: what does

private HttpHelper() { }

serve for? Do you want to forbid to create an instance of your class? If so, your class may be extended and instantiated, or your constructor may be accessed via reflection. Try this:

private HttpHelper() { 
    throw new NonInstantiableException() 
}

Answer 2

1) Is it OK that I use alot of static methods like getHTML?

The problem with static methods is that it's hard to unit test the other classes that use these static methods. But that's necessarily a huge problem in all use cases.

If you want to make your implementation more flexible and extensible, then yes, you may want to convert HttpHelper to an interface, and create an implementation of it (with non-static methods), and use an instance of your HttpHelperImpl in your class.

Actually, static methods are good if they are pure functions, with no internal state. But in your case you want the methods to reuse an http client instance, that makes the class have internal state. This makes the use of static methods arguable, because they have an external dependency: the contained http client instance.

2) Should I be implementing the HttpHelper class as singleton and use non static methods like HttpHelper.getInstance().getHTML(); (where the client is created in the getInstance method of the HttpHelper?

Perhaps it's not so much HttpHelper but the http client would be good as a singleton. Consider this approach:

class HttpHelperImpl implements HttpHelper {
    private final HttpClient client;

    public HttpHelperImpl() {
        // default setup, used by the application
        this(HttpClientSingleton.INSTANCE.getHttpClient());
    }

    public HttpHelperImpl(HttpClient client) {
        // called by the default constructor, but also usable by unit tests
        this.client = client;
    }

    public String getHTML(String cookie, String URL) throws IOException {
        // ...
        return null;
    }
}

enum HttpClientSingleton {
    INSTANCE;

    private final HttpClient httpClient;

    private HttpClientSingleton() {
        httpClient = new CustomHttpClient();
        httpClient.setConnectTimeout(TIMEOUT, TimeUnit.SECONDS);
    }

    HttpClient getHttpClient() {
        return httpClient;
    }
}