Reusable Implementation of the Publish-Subscribe Pattern

Question

I've finished an implementation of what I believe the Publish-Subscribe pattern is for practice and possible use in personal projects even though this implementation is fairly similar to the Observer/Observable classes of the Java API.

I'm looking for a review of my code-style, JavaDoc usage, API usage, implementation of the pattern, tests, and just about anything else you can think of.

Radio:

import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.Set;

public class Radio <Data> {
    /** A {@link HashMap} of events mapped to listening {@link Receiver Receivers}. */
    private final HashMap<String, Set<Receiver<Data>>> receivers = new HashMap<>();

    /**
    * Transmits an event without data.
    *
    * @param event
    *         The event to transmit.
    */
    public final void transmit(final String event) {
        if (event == null || event.isEmpty()) {
            throw new NullPointerException("A transmitted event cannot be null or empty.");
        }

        transmit(event, null);
    }

    /**
    * Transmits an event with data.
    *
    * @param event
    *         The event whose receivers are to be transmitted to.
    *
    * @param data
    *         The data to transmit to the {@link Receiver Receivers}.
    */
    public final void transmit(final String event, final Data data) {
        if (event == null || event.isEmpty()) {
            throw new NullPointerException("The event cannot be null or empty.");
        }

        final Set<Receiver<Data>> receivers = this.receivers.get(event);

        if (receivers != null) {
            receivers.forEach(receiver -> receiver.receive(event, data));
        }
    }

    /**
    * Adds a receiver to an event.
    *
    * @param event
    *         The event to add a receiver to.
    *
    * @param receiver
    *         The receiver to add.
    */
    public final void addReceiver(final String event, final Receiver<Data> receiver) {
        if (event == null || event.isEmpty()) {
            throw new NullPointerException("The event cannot be null or empty.");
        }

        if (receiver == null) {
            throw new NullPointerException("The receiver cannot be null.");
        }

        if (receivers.containsKey(event) == false) {
            receivers.put(event, new LinkedHashSet<>());
        }

        receivers.get(event).add(receiver);
    }

    /**
    * Removes a receiver from an event.
    *
    * @param event
    *         The event to remove a receiver from.
    *
    * @param receiver
    *         The receiver to remove.
    */
    public final void removeReceiver(final String event, final Receiver<Data> receiver) {
        if (event == null || event.isEmpty()) {
            throw new NullPointerException("The event cannot be null or empty.");
        }

        if (receiver == null) {
            throw new NullPointerException("The receiver cannot be null.");
        }

        final Set<Receiver<Data>> receivers = this.receivers.get(event);

        if (receivers != null) {
            receivers.remove(receiver);
        }
    }

    /**
    * Removes all receivers from an event.
    *
    * @param event
    *         The event to remove receivers from.
    */
    public final void removeReceivers(final String event) {
        if (event == null || event.isEmpty()) {
            throw new NullPointerException("The event cannot be null or empty.");
        }

        final Set<Receiver<Data>> receivers = this.receivers.get(event);

        if (receivers != null) {
            receivers.clear();
        }
    }
}

Receiver:

public interface Receiver<Data> {
    /**
    * Receives data from a transmission.
    *
    * @param event
    *         The event that caused the transmission.
    *
    * @param data
    *         The received data.
    */
    void receive(final String event, final Data data);
}

RadioTest:

import org.junit.Assert;
import org.junit.Test;

public class RadioTest {
    @Test
    public void transmitA() {
        final Radio<String> radio = new Radio<>();
        final ReceiverTest receiver = new ReceiverTest();

        radio.addReceiver("Test Event", receiver);
        radio.transmit("Test Event");

        Assert.assertEquals(receiver.getData(), null);
    }

    @Test
    public void transmitB() {
        final Radio<String> radio = new Radio<>();
        final ReceiverTest receiver = new ReceiverTest();

        radio.addReceiver("Test Event", receiver);
        radio.transmit("Test Event", null);

        Assert.assertEquals(receiver.getData(), null);
    }

    @Test
    public void transmitC() {
        final Radio<String> radio = new Radio<>();
        final ReceiverTest receiver = new ReceiverTest();

        radio.addReceiver("Test Event", receiver);
        radio.transmit("Test Event", "Hello");

        Assert.assertEquals(receiver.getData(), "Hello");
    }

    @Test
    public void transmitD() {
        final Radio<String> radio = new Radio<>();
        final ReceiverTest receiver = new ReceiverTest();

        radio.addReceiver("Test Event", receiver);
        radio.transmit("Other Event", "Hello");

        Assert.assertEquals(receiver.getData(), "");
    }

    @Test(expected=NullPointerException.class)
    public void removeReceiverA() {
        final Radio<String> radio = new Radio<>();
        radio.removeReceiver(null, null);
    }

    @Test(expected=NullPointerException.class)
    public void removeReceiverB() {
        final Radio<String> radio = new Radio<>();
        radio.removeReceiver("Test Event", null);
    }

    @Test
    public void removeReceiverC() {
        final Radio<String> radio = new Radio<>();
        final ReceiverTest receiver = new ReceiverTest();

        radio.removeReceiver("Test Event", receiver);
    }

    @Test
    public void removeReceiverD() {
        final Radio<String> radio = new Radio<>();
        final ReceiverTest receiver = new ReceiverTest();

        radio.addReceiver("Test Event", receiver);
        radio.removeReceiver("Test Event", receiver);
    }

    @Test(expected=NullPointerException.class)
    public void removeReceiversA() {
        final Radio<String> radio = new Radio<>();
        radio.removeReceivers(null);
    }

    @Test
    public void removeReceiversB() {
        final Radio<String> radio = new Radio<>();
        radio.removeReceivers("Test Event");
    }

    @Test
    public void removeReceiversC() {
        final Radio<String> radio = new Radio<>();
        final ReceiverTest receiver = new ReceiverTest();

        radio.addReceiver("Test Event", receiver);
        radio.removeReceivers("Test Event");
    }
}

ReceiverTest:

public class ReceiverTest implements Receiver<String> {
    private String data = "";

    @Override
    public void receive(final String event, final String data) {
        this.data = data;
    }

    public String getData() {
        return data;
    }
}

Answer 1

Receiver

• Data is a placeholder denoting a generic type. In Java, generics are pointed out with a single-letter literal (most often, T, E or U). The choice of Data is a bit misleading: one can easily understand that there is a type called Data, but indeed it is generic. Use a single letter name istead.

• final keywords with arguments in interface method declarations do not have much sense: the implementor may declare them as non final, it's up to him to decide. So you can avoid extra verbosity and declare the method as void receive(String event, T data);.

Radio

• Same remark about the generic type: it's better to use a single-letter literal.

• The receivers field should be typed as Map (interface) instead of HashMap (implementation). Prefer interfaces to implementations where possible, especially for collections.

• There are possible name clashes in the calls final Set<Receiver<Data>> receivers = this.receivers.get(event);. Naming a local variable as a field of the parent object is not a crime, but is prone to errors and bugs.

• if (receivers.containsKey(event) == false) is the same thing as if (!receivers.containsKey(event)), but consider using the dedicated receivers.putIfAbsent instead of calling containsKey.

Args Validation

It is not optimal for many of the methods:

• there are code duplications. The checks should be extracted to a method like ensureStringArgNotNullNotEmpty(String arg, String argName), which should be able to produce an exception message with the name of the argument.

• event.isEmpty() does not correspond to the meaning of the thrown exception. event is not null here, so why NullPointerException ?

I think that IllegalArgumentException is more appropriate for these validation checks.

If you still want to remain on NullPointerException, consider using one-liners with the standard Objects.requireNonNull call.

RadioTest

All the tests are clean, nicely coded and respect the principle of one assertion per test case.

But

1. They are all poorly named.

Just compare the current naming:

transmitA();
transmitB();

with alternatives:

nullDataReturnedWhenTransmittingEventOnly();
nullDataReturnedWhenTransmittingEventWithNullData();

If something breaks, you'll easily find where the problem is, instead of decoding the meaning of A or B.

2. The tests methods named transmit* make assertions about Receiver instances. Shouldn't they be placed in ReceiverJUnitTest cases ?

Answer 2

2 minor comments:

• Your code style is fairly "aggressive" (the technical term is offensive) in the sense that you're throwing an exception as soon as an error occurred. Especially for a pub/sub piece, which I would consider more infrastructure than client-side, I would be using more of a defensive programming approach.

• Pub/Sub usually means multi-threaded, in which case you should consider using the following classes (as well as the correct CAS/or atomic methods for those methods):

  • a ConcurrentHashMap instead of a HashMap
  • a ConcurrentSkipListSet instead of a LinkedHashSet