5
\$\begingroup\$

In my Java event bus project, I have a private generic method that retrieves a Set<Subscriber<E>> from a private Map<Class<? extends Event>, Set<Subscriber<? extends Event>>> (the subscriberMap).

/**
 * A map of Event types and Subscriber Sets.
 * 
 * That is, the Set of Subscribers set to an Event type
 * is assumed to be used for that Event type.
 */
private final Map<Class<? extends Event>, Set<Subscriber<? extends Event>>> subscriberMap = new ConcurrentHashMap<>();

private <E extends Event> void checkSubscriberMap(Class<E> type)
{
    if (!subscriberMap.containsKey(type))
    {
        subscriberMap.put(type, new HashSet<>());
    }
}

/**
 * Gets a Set of Subscribers by an Event.
 *
 * @param event the event
 * @param <E> the type of event
 * @return a Set of Subscribers
 */
@SuppressWarnings("unchecked")
private <E extends Event> Set<Subscriber<E>> getSubscribers(E event)
{
    return (Set<Subscriber<E>>) (Object) getSubscribers(event.getClass());
}

/**
 * Gets a Set of Subscribers by an Event type.
 *
 * @param type the event class
 * @param <E> the type of event
 * @return a Set of Subscribers
 */
@SuppressWarnings("unchecked")
private <E extends Event> Set<Subscriber<E>> getSubscribers(Class<E> type)
{
    checkSubscriberMap(type); // checks if subscriberMap.get(type) is null

    return (Set<Subscriber<E>>) (Object) subscriberMap.get(type);
}

Assuming nobody uses reflection to modify subscriberMap, the public methods that modify subscriberMap only work if the Subscriber<? extends Event> is the same type as the Class<? extends Event>. Therefore, I can be sure that getSubscribers() will always return a Set of the same type that you give it.

/**
 * Registers a single event handler under an explicit Event type.
 * Returns true if the handler was added.
 *
 * @param subscriber the Subscriber
 * @param type the type of Event
 */
public <E extends Event> boolean register(Subscriber<E> subscriber, Class<E> type)
{
    if(type == Event.class)
    {
        throw new IllegalArgumentException("The provided type is not a subclass of Event.");
    }

    return getSubscribers(type).add(subscriber);
}

However, it's bothering me how getSubscribers() looks. Why does it look I'm doing a hacky workaround? Is it necessary that I have to cast subscriberMap.get(type) twice? Is this the right way to go about what I'm trying to do?

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$
private <E extends Event> void checkSubscriberMap(Class<E> type)

This method checks nothing. It ensures that there's a non-null value. Or it fixes or prepares something.

Note that you're using both contains and get every time. I guess, a method like

private <E extends Event> Set<E> safeGet(Class<E> type) {
   Set<E> result =
           (Set<Subscriber<E>>) (Object) subscriberMap.get(type);
   if (result == null) {
        result = new HashSet<>();
        subscriberMap.put(type, result);
   }
   return result;
}

would be better.

Consider using Guava's Multimap as it's been solved there.


@SuppressWarnings("unchecked")
private <E extends Event> Set<Subscriber<E>> getSubscribers(E event)
{
    return (Set<Subscriber<E>>) (Object) getSubscribers(event.getClass());
}

I'm afraid, that's right. The ugly double cast is needed as there's no way to tell the compiler that it works. Maybe the intermediate cast could be (non-generic) (Set).


Assuming nobody uses reflection

If somebody does, that's not your fault.

What you did is similar in idea to the MutableClassToInstanceMap, except that you need Set<Subscriber<E>> instead of E itself and therefore you can't use type.cast(object) and have to resolve to an unchecked cast.

\$\endgroup\$
2
  • \$\begingroup\$ I appreciate this detailed answer, but could you please elaborate on what a proper subclass is? That's really the only thing I don't fully understand. The type == Event.class check is because I only want subclasses of Event.class in subscriberMap. Should I check Event.class.isAssignableFrom(type) as well? \$\endgroup\$
    – octopod
    May 22, 2015 at 11:52
  • \$\begingroup\$ @Octopod I meant proper subclass like proper subset, but it look like I was wrong. Removed. No need for isAssignableFrom, just maybe a better formulation (maybe "It's not allowed to register a subscriber for the Event class itself."). Or allow it. Or let it be. \$\endgroup\$
    – maaartinus
    May 22, 2015 at 20:27
2
\$\begingroup\$

Use your tools properly ;-) You are missing a bunch of functionality that is available on your class that would help you a lot.

First up, you have the following:

private final Map<Class<? extends Event>, Set<Subscriber<? extends Event>>> subscriberMap = new ConcurrentHashMap<>();

Now, that's a long line, and as a result, it took me a moment to discover that this is a ConcurrentHashMap ... this makes a big difference. So, let's use that to our advantage:

private final ConcurrentMap<Class<? extends Event>,
                            Set<Subscriber<? extends Event>>> subscriberMap
     = new ConcurrentHashMap<>();

I have exaggerated the indentation to make it all visible.....

Now, ConcurrentMap gives us some extra methods over just-plain-Map... You should use them.

First up, this "check" method. @maaartinus correctly identifies this as being a problem with the name. The standard name used in many libraries (including the Java Native libraries, is ensure......

private <E extends Event> void checkSubscriberMap(Class<E> type)
{
    if (!subscriberMap.containsKey(type))
    {
        subscriberMap.put(type, new HashSet<>());
    }
}

I would write that as:

private <E extends Event> void ensureSubscriberMap(Class<E> type) {
    subscriberMap.computeIfAbsent(type, k -> new HashSet<>());
}

Note that the supply-function is only called if the key is absent. Also, note that this is now an atomic operation, there is no race condition at all.

Now, looking through your additional code, I am perturbed by the lack of defensive copies..... The Map is concurrent, so I suspect that you have concurrency concerns... what if some other thread modifies the Subscriber sets part-way through operation? You may have a real problem. Defensive copies of data structures are normally recommended for this, but your base structure is just a HashSet, it's not thread safe. So, I would recommend a lock on that of some sort, with a thread-safe copy... also, since the 'ensure' method is now a 1-liner, I have included it directly in this method:

@SuppressWarnings("unchecked")
private <E extends Event> Set<Subscriber<E>> getSubscribers(Class<E> type) {

    // take a reference to the non-specific-generic Set 
    Set<Subscriber<? extends Event>> subs = 
             subscriberMap.computeIfAbsent(type, k -> new HashSet<>());

    // lock that set so only we can traverse it (thread safe)
    synchronized (subs) {

        // map each member to a defensive copy using the specific generic mapping.
        return subs.stream()
                   .map(s -> (Subscriber<E>)s)
                   .collect(Collectors.toSet());

    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Heh, sorry about that long line. I'm not used to cutting a single line into multiple. Also, although I could always look it up myself, could you elaborate on what subs.stream().map().collect() does? I think that would help me out a lot. \$\endgroup\$
    – octopod
    May 22, 2015 at 12:00
  • \$\begingroup\$ @Octopod In this case, it's probably better for you to look it up yourself -- it'll provide a great intro to Java 8's new streams, which are a bit part of it, and if you look it up yourself, you'll be getting more general knowledge of it. (and if you're just confused about the specific functions, it'll help you learn them :D) \$\endgroup\$
    – anon
    May 30, 2015 at 16:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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