3
\$\begingroup\$

Suppose I am listening for network changes with the following listener:

interface NetListener {

    void onNetworkAvailable(boolean isWifi);

    void onNetworkUnavailable();
}

I want to have an object, accessible from anywhere within the app, holding the registered NetListeners, that can trigger the appropriate methods when necessary. The solution I came up with is this "singleton":

class NetListenerManager {

    private static List<NetListener> _listeners = new CopyOnWriteArrayList<NetListener>();

    static void addListener(final NetListener listener) {
        if (null != listener) { _listeners.add(listener); }
    }

    static void removeListener(final NetListener listener) {
        if (null != listener) { _listeners.remove(listener); }
    }

    /**
     * Called when a network connection is available.
     */
    static void notifyOnNetworkAvailable(boolean isWifi) {
        for (NetListener listener : _listeners) { listener.onNetworkAvailable(isWifi); }
    }

    /**
     * Called when a network connection becomes unavailable.
     */
    static void notifyOnNetworkUnavailable() {
        for (NetListener listener : _listeners) { listener.onNetworkUnavailable(); }
    }

    private NetListenerManager() { /* Instantiation disabled. */ }

}

So whenever e.g. a network change broadcast is received, I call NetListenerManager.notifyOnNetworkAvailable or NetListenerManager.notifyOnNetworkUnavailable, depending on the network state.

I’m wondering if this is a good solution or if there are better ways to implement this. Thanks!

UPDATE no.2

This is the modified listener:

interface NetListener {

    static enum NetworkType {NO_NETWORK, WIFI, MOBILE}

    void onNetworkChange(NetworkType type);

}

And the manager:

public final class NetListenerManager {

    private static final List<NetListener> NET_LISTENERS = new CopyOnWriteArrayList<NetListener>();
    private static final BlockingQueue<NetListener.NetworkType> QUEUE = new LinkedBlockingQueue<NetListener.NetworkType>();

    private static final Thread QUEUE_PROCESSOR = new Thread(new QueueProcessor());
    static { QUEUE_PROCESSOR.start(); }


    public static void addListener(final NetListener listener) {
        if (null != listener) { NET_LISTENERS.add(listener); }
    }

    public static void removeListener(final NetListener listener) {
        if (null != listener) { NET_LISTENERS.remove(listener); }
    }

    /**
     * Called when there is a network change.
     */
    public static void notifyOnNetworkChange(final NetListener.NetworkType type) {
        QUEUE.add(type);
    }

    private NetListenerManager() { /* Instantiation disabled. */ }

    /**
     * This Runnable propagates network change events to the registered NetListener listeners in a queue.
     */
    private static final class QueueProcessor implements Runnable {
        @Override
        public void run() {
            while (true) {
                try {
                    final NetListener.NetworkType type = QUEUE.take();
                    for (NetListener listener : NET_LISTENERS) { listener.onNetworkChange(type); }
                } catch (InterruptedException ie) { /* Do nothing */ }
            }
        }
    }

}
\$\endgroup\$
  • \$\begingroup\$ Can a listener trigger a change in the network availability and therefore re-trigger the event while it is being processed? \$\endgroup\$ – Paolo Brandoli Nov 26 '13 at 10:59
  • \$\begingroup\$ Consider using en enum for Singleton designs, as proposed by Joshua Blochs in Effective Java. (ex: stackoverflow.com/a/71399/1021726) \$\endgroup\$ – Max Nov 26 '13 at 11:51
  • 1
    \$\begingroup\$ I have updated my answer to match your revised questions. \$\endgroup\$ – rolfl Nov 26 '13 at 13:18
  • \$\begingroup\$ Why evil singleton? \$\endgroup\$ – abuzittin gillifirca Nov 27 '13 at 11:09
2
\$\begingroup\$

In general this is a good solution, but there are a few points you should consider:

  • General style: Make the class final. There is no reason for anyone to subclass it, and the private Constructor shows that intent, but making the class itself final is a good thing.
  • Similarly, make the _listeners list final as well.

The next item is whether you can guarantee that the static methods on your class will only ever be called from a single thread. If you can guarantee this then the code will be good. If not, then it is possible for there to be 'transient' situations where the network 'blips', sending a 'network down' followed by a 'network up' notification. If they come from different threads then it is possible the the NetListener instances may get the notifications out of order.

For a thread-safe system I would create an enum with three states:

enum (NET_DOWN, NET_UP_WIFI, NET_UP_REGULAR);

and then feed those states on to a Concurrent*Queue on the notify side. Then, have a seperate thread that reads from that queue (guaranteeing the order) and feeds the events to the listeners.

This also removes potentially long-running method calls from the service that provides you the notification events.... (i.e. this may make the OS more responsive to other applications)


EDIT

After your edit you have introduced the concept of a separate thread for managing the notification process.

The only comment I have for the modification is that the java.util.concurrent typically has 'the right tool for the job' somewhere. In this case, you chose the wrong tool. It is pretty complicated sometimes, but the right tool would probably be an implementation of a BlockingQueue. In this case I would choose LinkedBlockingQueue. Your methods can then lose the synchronization blocks and simply become:

public static void notifyOnNetworkChange(final NetListener.NetworkType type) {
    QUEUE.put(type);
}

and your read-side becomes (note you had a bug before because if you had multiple listeners you would poll different values for each of them):

while(...) {
    NetworkType state = QUEUE.take();
    for (NetListener listener : NET_LISTENERS) { listener.onNetworkChange(state);

}

The LinkedBlockingQueue deals with all the synchronization issues for you.

\$\endgroup\$
  • \$\begingroup\$ I think your enum types are named wrong. DOWN and OTHER are, as you said, states. WIFI on the other hand is a network type, and WIFI as a network can be DOWN, RUNNING, OTHER, etc. \$\endgroup\$ – Max Nov 26 '13 at 11:53
  • \$\begingroup\$ The three states relate to the three possible method calls to the handler... but you are right, the names are poor.... editing. \$\endgroup\$ – rolfl Nov 26 '13 at 12:11
  • \$\begingroup\$ Hi guys, thank you very much for your feedback. I've updated my post, could you please take a look? Any feedback highly appreciated! Thanks again. \$\endgroup\$ – Tadej Nov 26 '13 at 13:05
4
\$\begingroup\$

(Disclaimer: I posted this as a comment first but feel like it should also be an answer)

You should consider to use enum as a singleton as proposed by Joshua Blochs in Effective Java. This will protect you from both serialization attacks and reflection attacks on your singleton instance.

Here is a post which discusses it a lot more in-depth. https://stackoverflow.com/questions/70689/what-is-an-efficient-way-to-implement-a-singleton-pattern-in-java/71399#71399

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.