snippetjavaMinor
Correct way to implement a Singleton field
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.
I am using methods like
-
Is it OK that I use a lot of static methods like
-
Should I be implementing the
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 (
A little suggestion:
if you initailize your client in this way, you won't need
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
Addition:
what does
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:
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.