HiveBrain v1.2.0
Get Started
← Back to all entries
snippetjavaMinor

Correct way to implement a Singleton field

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fieldimplementwaysingletoncorrect

Problem

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.

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

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

Solution

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

Code Snippets

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

Context

StackExchange Code Review Q#62010, answer score: 2

Revisions (0)

No revisions yet.