Your class is more or less. But the first big problem I see is the builder being public. With that, someone just invoke it and your singleton will no longer be a singleton.
There is another big problem too: Synchronization. Just two Threads
invoke getInstance()
or setContext(Context)
at the same time and with a little luck you can end up with two singleton instances, or with two different contexts.
In addition, there is an important question: Why instantiate the class Global
in lazy ? It has no heavy resources to be instantiated in the constructor. With this your class looks like this:
public class Global {
private static final Global instance = new Global();
private Context context;
private Global() {
}
public static Global getInstance() {
return instance;
}
public synchronized Context getContext() {
return context;
}
public synchronized void setContext(Context context) {
this.context = context;
}
}
And so it's already cool, especially if you want to add more things to your singleton.
Something that worries me a bit is this setContext()
. Ideally, your singleton is immutable, and once properly configured, it will never be tweaked. But before that comes the question: How do you get Context
? There are several contexts in android, what context exactly are you storing in your singleton and why do you need to do this?
In addition, depending on the case (not in every singleton), you can leave your static methods, eliminating the need to have getInstance()
:
public class Global {
private static final Global instance = new Global();
private Context context;
private Global() {
}
public static synchronized Context getContext() {
return instance.context;
}
public static synchronized void setContext(Context context) {
instance.context = context;
}
}
And then you would only use Global.getContext();
instead of Global.getInstance().getContext();
. Also, you would never see an instance of Global
outside of the Global
class itself (which is fine since it probably would not make sense).
Returning to the multithreading issue, to avoid having the synchronized
methods you can make the instance variable context
be volatile
:
public class Global {
private static final Global instance = new Global();
private volatile Context context;
private Global() {
}
public static Context getContext() {
return instance.context;
}
public static void setContext(Context context) {
instance.context = context;
}
}
Another possibility is to use AtomicReference<Context>
as the class field:
public class Global {
private static final Global instance = new Global();
private final AtomicReference<Context> contextRef = new AtomicReference<>();
private Global() {
}
public static Context getContext() {
return instance.contextRef.get();
}
public static void setContext(Context context) {
instance.contextRef.set(context);
}
}