3
\$\begingroup\$

I am new to Java and had some difficulty creating this simple Publisher Subscriber class. It finally seems to be working and I wanted to check if I have been following best practices and see if there are any improvements I can make.

The following Java class accepts any topics as a string (e.g. "rand_int") and forwards any messages to that topic to any subscribers.

import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Consumer;

public class PubSub {
  private Map<String, ArrayList<Consumer<?>>> topicsSubscribers =
      new HashMap<String, ArrayList<Consumer<?>>>();
  private static PubSub pubSubSingleton = null;

  public static PubSub getInstance() {
    if (pubSubSingleton == null)
      pubSubSingleton = new PubSub();

    return pubSubSingleton;
  }

  public <T> void publish(String topic, T message) {
    ArrayList<Consumer<?>> subscribers = this.topicsSubscribers.get(topic);
    if (subscribers == null)
      return;

    for (Consumer subscriberConsumer : subscribers) {
      subscriberConsumer.accept(message);
    }
  }

  public synchronized <T> void subscribe(String topicName, Consumer<T> subscriberCallback) {
    ArrayList<Consumer<?>> subscribers = this.topicsSubscribers.get(topicName);
    if (subscribers == null) {
      subscribers = new ArrayList<Consumer<?>>();
      subscribers.add(subscriberCallback);
      this.topicsSubscribers.put(topicName, subscribers);
    } else {
      subscribers.add(subscriberCallback);
    }
  }
}

Example usage:

public class SomeClass {
  public SomeClass() {
    PubSub.getInstance().subscribe("rand_int", this::randIntCallback);
    PubSub.getInstance().publish("rand_int", 46);
  }

  public randIntCallback(int number) {
    System.out.println(number);
  }
}

The above code should output "46".

As I said, I am very new to Java and would like some feedback on my code to help improve. So any help on this is very appreciated.

\$\endgroup\$
  • 1
    \$\begingroup\$ Your error message is off topic for this site. If you pared it down to an MCVE, it would be on-topic on Stack Overflow. But it would likely get closed as a duplicate of a question like this. \$\endgroup\$ – mdfst13 Dec 14 '18 at 17:23
  • \$\begingroup\$ Thanks mdfst13. Yes I only kept the error as the reason I wasn't able to improve my code a certain way. But I suppose you are right it's a bit off topic. Thanks for your answer as well. All very good points which I will look into and implement as much as possible. \$\endgroup\$ – KNejad Dec 14 '18 at 17:36
  • \$\begingroup\$ You should give others some time to write an answer too before you accept the first answer you get. \$\endgroup\$ – Emily L. Dec 14 '18 at 22:12
  • \$\begingroup\$ @EmilyL. I originally was going to wait but figured if a better answer comes along I can just change the accepted answer. Is that not possible? \$\endgroup\$ – KNejad Dec 15 '18 at 17:01
  • \$\begingroup\$ Sure but it discourages others from answering. \$\endgroup\$ – Emily L. Dec 15 '18 at 17:04
3
\$\begingroup\$

Initializing object variables

  private Map<String, ArrayList<Consumer<?>>> topicsSubscribers =
      new HashMap<String, ArrayList<Consumer<?>>>();

The standard is to set the types as the interfaces, like Map and List.

    private final Map<String, List<Consumer<?>>> topicSubscribers = new HashMap<>();

Also, you don't need to specify the contents of <> in the implementation instantiation. The compiler will work that out for you.

Initializing class variables

  private static PubSub pubSubSingleton = null;

  public static PubSub getInstance() {
    if (pubSubSingleton == null)
      pubSubSingleton = new PubSub();

    return pubSubSingleton;
  }

You don't have to do it this way. You can simply say

    private static final PubSub pubSubSingleton = new PubSub();

    public static PubSub getInstance() {
        return pubSubSingleton;
    }

    private PubSub() {
    }

This saves the null check each time while still only initializing once.

However, Singleton is generally considered an anti-pattern now. You may want to consider alternatives. One possibility would be a separate instance for each message type.

Your original code does not make the constructor private.

The Java standard is four column indents rather than the two you are using.

Formatting

    if (subscribers == null)
      return;

If you are going to use the statement form, you should probably keep it one line:

    if (subscribers == null) return;

If it's too complicated to fit into one line, you should probably use the block form instead.

I actually use the block form consistently regardless.

    if (subscribers == null) {
        return;
    }

Factor out the common

    if (subscribers == null) {
      subscribers = new ArrayList<Consumer<?>>();
      subscribers.add(subscriberCallback);
      this.topicsSubscribers.put(topicName, subscribers);
    } else {
      subscribers.add(subscriberCallback);
    }

This could be

    if (subscribers == null) {
        subscribers = new ArrayList<Consumer<?>>();
        topicsSubscribers.put(topicName, subscribers);
    }

    subscribers.add(subscriberCallback);

You don't have to do the add before the put. It's the same object either way. Otherwise, you'd have to do the put after the add in the non-null case.

\$\endgroup\$
2
\$\begingroup\$

We consider singletons bad (i.e. your getInstance bit). There are numerous resources on why a short search away so I'm not going to list them here.

That said your getInstance is not safe in a threaded context. You need to use Double-checked locking. Note that I do think that initialising the object in the static constructor or inline with the declaration is safe as well.

Your subscribe method is synchronized on the class object but the publish method is not meaning that you have a data race where the subscribers for a topic may change in the middle of iterating over it, typically this will throw a ConcurrentModificationException when it happens. Either you do a thread safe design and add synchronized to publish as well or do a thread-un-safe design and remove the synchronized keyword everywhere.

Your publish method swallows any errors. This is not good. Depending on your philosophy on error reporting you should either return a boolean signifying the success or failure of publishing the message to at least one receiver or throw an exception. In this case I would return an error, or maybe even the number of subscribers that the message was delivered too, yeah I think that makes more sense and is cheap to compute and rich in information.

And finally, your class will forever hold a reference to the subscriber object and is likely to cause a memory leak as you cannot remove them, especially since this is a singleton which has the same lifetime as your application. A common pattern is to either allow removal through unsubscribe or use WeakReference and let the life time be tied to an external object with cleanup of any weak references whose get returns null (object removed/gcd)

\$\endgroup\$
1
\$\begingroup\$

One small addition to what the others said:

ArrayList<Consumer<?>> subscribers = this.topicsSubscribers.get(topicName);
if (subscribers == null) {
  subscribers = new ArrayList<Consumer<?>>();
  subscribers.add(subscriberCallback);
  ...

is an outdated pattern. Since java 8 (i.e. 4.5 years now) we have computeIfAbsent in the Map. Therefore, use:

ArrayList<Consumer<?>> subscribers = this.topicsSubscribers.computeIfAbsent(sub -> new ArrayList<>());

instead.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the tip. But wouldn't putIfAbsent () be more appropriate here? \$\endgroup\$ – KNejad Dec 16 '18 at 14:59
  • 1
    \$\begingroup\$ @KNejad No. For putIfAbsent you'd have to create a new list for every call which would be discarded in most cases. For computeIfAbsent you pass a supplier, which will only be called to create a new list if needed. \$\endgroup\$ – mtj Dec 17 '18 at 6:19

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.