patternjavaMinor
Is this a good implementation of a thread-safe singleton using the observer pattern?
Viewed 0 times
thistheobserversingletonthreadusingsafegoodimplementationpattern
Problem
I need a singleton that can safely operate in a multi-thread environment. Threading and concurrency is new to me, so I'm not sure if this implementation holds. Take a look:
Any feedback?
public class ObserverSingleton {
private static ObserverSingleton _instance;
private ArrayList _listeners;
private ObserverSingleton() {
_listeners = new ArrayList();
}
public static synchronized ObserverSingleton getInstance() {
if(_instance == null) {
_instance = new ObserverSingleton();
}
return _instance;
}
public synchronized void addOnRequestFinishedListener(OnRequestFinishedListener listener) {
// Check if listener already exists in list, skip if it does
for (OnRequestFinishedListener listListener : _listeners) {
if(listListener.equals(listener)) {
return;
}
}
_listeners.add(listener);
}
public synchronized void removeOnRequestFinishedListener(OnRequestFinishedListener listener) {
// Check if listener exists in list, remove if it does
for (OnRequestFinishedListener listListener : _listeners) {
if(listListener.equals(listener)) {
_listeners.remove(listListener);
}
}
}
private void notifyListeners() {
for (OnRequestFinishedListener listener : _listeners) {
listener.onRequestFinished();
}
}
public Object clone() throws CloneNotSupportedException {
throw new CloneNotSupportedException();
}
public interface OnRequestFinishedListener {
public void onRequestFinished();
}
}Any feedback?
Solution
The
Unless lazy instantiation is really needed, I wouldn't use it. Just use the following code:
Also, I would use a
Declaring the List as
notifyListeners() method is private, and is never called anywhere. It should be synchronized on the same lock than the other methods using the list of listeners.Unless lazy instantiation is really needed, I wouldn't use it. Just use the following code:
private static final ObserverSingleton INSTANCE = new ObserverSingleton();
public static ObserverSingleton getInstance() {
return INSTANCE;
}Also, I would use a
Set rather than a list, since your add method makes sure there is only one copy of a listener inside the list, and Set is precisely used for that. If order of insertion matters, use a LinkedHashSet. Declaring the List as
ArrayList is bad practice. You should declare it as List, since the concrete type doesn't matter.Code Snippets
private static final ObserverSingleton INSTANCE = new ObserverSingleton();
public static ObserverSingleton getInstance() {
return INSTANCE;
}Context
StackExchange Code Review Q#12484, answer score: 6
Revisions (0)
No revisions yet.