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

Is this a good implementation of a thread-safe singleton using the observer pattern?

Submitted by: @import:stackexchange-codereview··
0
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:

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