3
\$\begingroup\$

In my MUD game engine, I built a messaging api that allows objects to subscribe to concrete implementations of an IMessage interface. Is there anything glaringly bad about the approach I have taken?

The intended use-case is for objects, such as an IPlayer implementation, to subscribe to WhisperMessage and ShoutMessage publishes. When a player sends a chat message, a mediator performs a Publish for an instance of ShoutMessage and all IPlayer implementations that subscribed are given the message.

IMessage, it's generic counterpart, base class and implementation

public interface IMessage
{
    /// <summary>
    /// Gets the content.
    /// </summary>
    /// <returns>Returns the content of the message</returns>
    object GetContent();
}    

/// <summary>
/// Allows for receiving the content of a message
/// </summary>
/// <typeparam name="TContent">The type of the content.</typeparam>
public interface IMessage<TContent> : IMessage where TContent : class
{
    /// <summary>
    /// Gets the content of the message.
    /// </summary>
    TContent Content { get; }

    /// <summary>
    /// Gets the content of the message.
    /// </summary>
    /// <returns>Returns the message content</returns>
    new TContent GetContent();
}

public abstract class MessageBase<TContentType> : IMessage<TContentType> where TContentType : class
{
    /// <summary>
    /// Gets the content of the message.
    /// </summary>
    public TContentType Content { get; protected set; }

    /// <summary>
    /// Gets the content of the message.
    /// </summary>
    /// <returns>
    /// Returns the message content
    /// </returns>
    public TContentType GetContent()
    {
        return this.Content;
    }

    /// <summary>
    /// Gets the content.
    /// </summary>
    /// <returns>
    /// Returns the content of the message
    /// </returns>
    object IMessage.GetContent()
    {
        return this.GetContent();
    }
}

public class ShoutMessage : MessageBase<string>
{
    /// <summary>
    /// Initializes a new instance of the <see cref="ShoutMessage"/> class.
    /// </summary>
    /// <param name="message">The message.</param>
    public ShoutMessage(string message)
    {
        this.Content = message;
    }
}

/// <summary>
/// Used when an object needs to send a private message to a character.
/// </summary>
public class WhisperMessage : MessageBase<string>
{
    /// <summary>
    /// Initializes a new instance of the <see cref="WhisperMessage"/> class.
    /// </summary>
    /// <param name="message">The message.</param>
    /// <param name="target">The target.</param>
    public WhisperMessage(string message, ICharacter target)
    {
        this.Content = message;
        this.Target = target;
    }

    /// <summary>
    /// Gets the target that this message is intended for.
    /// </summary>
    public ICharacter Target { get; private set; }
}

Notification handlers

public interface ISubscription
{
    /// <summary>
    /// Occurs when the subscription is being unsubscribed.
    /// </summary>
    event Action<NotificationArgs> Unsubscribing;

    /// <summary>
    /// Gets or sets a value indicating whether this <see cref="ISubscription"/> is active.
    /// </summary>
    bool IsActive { get; }

    /// <summary>
    /// Unsubscribes the registerd callbacks from receiving notifications.
    /// </summary>
    /// <param name="notificationCenter">The notification center.</param>
    void Unsubscribe();
}

public interface INotification<TMessageType> : ISubscription where TMessageType : class, IMessage
{
    /// <summary>
    /// Registers the specified action for callback when a notification is fired for T.
    /// </summary>
    /// <param name="callback">The message being posted along with the subscription registered to receive the post.</param>
    /// <returns></returns>
    void Register(
        Action<TMessageType, ISubscription> callback,
        Func<TMessageType, bool> condition = null);

    /// <summary>
    /// Processes the message, invoking the registered callbacks if their conditions are met.
    /// </summary>
    /// <param name="message">The message.</param>
    void ProcessMessage(TMessageType message);
}

internal class Notification<TMessage> : INotification<TMessage> where TMessage : class, IMessage
{
    /// <summary>
    /// The callbacks invoked when the handler processes the messages.
    /// </summary>
    private Action<TMessage, ISubscription> callback;

    /// <summary>
    /// The conditions that must be met in order to fire the callbacks.
    /// </summary>
    private Func<TMessage, bool> condition;

    /// <summary>
    /// Occurs when the subscription is being unsubscribed.
    /// </summary>
    public event Action<NotificationArgs> Unsubscribing;

    public bool IsActive { get; protected set; }

    /// <summary>
    /// Registers a callback for when a chat message is published by the MessageCenter
    /// </summary>
    /// <param name="processor">The message.</param>
    /// <returns></returns>
    public void Register(
        Action<TMessage, ISubscription> processor,
        Func<TMessage, bool> condition)
    {
        this.callback = processor;
        this.condition = condition;
        this.IsActive = true;
    }

    /// <summary>
    /// Unsubscribes the handler from notifications. This cleans up all of the callback references and conditions.
    /// </summary>
    public void Unsubscribe()
    {
        this.callback = null;
        this.condition = null;

        try
        {
            this.OnUnsubscribing();
        }
        // TODO: Write custom SubscriptionException to catch.
        finally
        {
            this.IsActive = false;
        }
    }

    /// <summary>
    /// Processes the message by verifying the callbacks can be invoked, then invoking them.
    /// </summary>
    /// <param name="message">The message.</param>
    public void ProcessMessage(TMessage message)
    {
        if (this.condition == null)
        {
            this.callback(message, this);
            return;
        }

        if (!this.condition(message))
        {
            return;
        }

        this.callback(message, this);
    }

    protected virtual void OnUnsubscribing()
    {
        var handler = this.Unsubscribing;
        if (handler == null)
        {
            return;
        }

        handler(new NotificationArgs(this, typeof(TMessage)));
    }
}

public class NotificationArgs
{
    public NotificationArgs(ISubscription subscription, Type messageType)
    {
        this.Subscription = subscription;
        this.MessageType = messageType;
    }

    public ISubscription Subscription { get; private set; }

    public Type MessageType { get; private set; }
}

NotificationManager

public interface INotificationCenter
{
    /// <summary>
    /// Sets up a new handler and returns it for subscription set up.
    /// </summary>
    /// <typeparam name="TMessageType">An IMessage implementation that the given handler will be provided when messages are dispatched</typeparam>
    /// <param name="handler">The handler used to process incoming messages.</param>
    /// <returns>Returns an ISubscription that can be used to unsubscribe.</returns>
    ISubscription Subscribe<TMessageType>(Action<TMessageType, ISubscription> callback, Func<TMessageType, bool> condition = null) where TMessageType : class, IMessage;

    /// <summary>
    /// Publishes the specified message.
    /// </summary>
    /// <param name="message">The message.</param>
    void Publish<TMessageType>(TMessageType message) where TMessageType : class, IMessage;
}

/// <summary>
/// The mediator for all messaging
/// </summary>
public class NotificationManager : INotificationCenter
{
    /// <summary>
    /// Collection of subscribed listeners
    /// </summary>
    private ConcurrentDictionary<Type, List<ISubscription>> listeners =
        new ConcurrentDictionary<Type, List<ISubscription>>();

    private object listenerCollectionLock = new object();

    /// <summary>
    /// Subscribe publications for the message type specified.
    /// </summary>
    /// <typeparam name="TMessageType"></typeparam>
    /// <returns></returns>
    public ISubscription Subscribe<TMessageType>(Action<TMessageType, ISubscription> callback, Func<TMessageType, bool> condition = null) where TMessageType : class, IMessage
    {
        Type messageType = typeof(TMessageType);

        // Create our key if it doesn't exist along with an empty collection as the value.
        if (!listeners.ContainsKey(messageType))
        {
            listeners.TryAdd(messageType, new List<ISubscription>());
        }

        // Add our notification to our listener collection so we can publish to it later, then return it.
        // TODO: Move instancing the Notification in to a Factory.
        var handler = new Notification<TMessageType>();
        handler.Register(callback, condition);
        handler.Unsubscribing += this.Unsubscribe;

        List<ISubscription> subscribers = listeners[messageType];
        lock (listenerCollectionLock)
        {
            subscribers.Add(handler);
        }

        return handler;
    }

    /// <summary>
    /// Publishes the specified message to all subscribers
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <param name="message">The message.</param>
    public void Publish<T>(T message) where T : class, IMessage
    {
        if (!listeners.ContainsKey(typeof(T)))
        {
            return;
        }

        foreach (INotification<T> handler in listeners[typeof(T)])
        {
            handler.ProcessMessage(message);
        }
    }

    /// <summary>
    /// Unsubscribes the specified handler by removing their handler from our collection.
    /// </summary>
    /// <typeparam name="T">The message Type you want to unsubscribe from</typeparam>
    /// <param name="subscription">The subscription to unsubscribe.</param>
    private void Unsubscribe(NotificationArgs args)
    {
        // If the key doesn't exist or has an empty collection we just return.
        // We will leave the key in there for future subscriptions to use.
        if (!listeners.ContainsKey(args.MessageType) || listeners[args.MessageType].Count == 0)
        {
            return;
        }

        // Remove the subscription from the collection associated with the key.
        List<ISubscription> subscribers = listeners[args.MessageType];
        lock (listenerCollectionLock)
        {
            subscribers.Remove(args.Subscription);
        }

        args.Subscription.Unsubscribing -= this.Unsubscribe;
    }
}

Unit Tests

The following unit tests show how I (and 3rd party devs using the engine) will use the API.

public class NotificationManagerTests
{
    [TestMethod]
    [TestCategory("Runtime.Game - NotificationManager")]
    public void Publish_invokes_callbacks()
    {
        bool callbackCalled = false;
        string messageContent = "Test";
        var notificationCenter = new NotificationManager();
        notificationCenter.Subscribe<ShoutMessage>((msg, sub) =>
            {
                if (msg.Content == messageContent)
                {
                    callbackCalled = true;
                }
            });

        // Act
        notificationCenter.Publish(new ShoutMessage(messageContent));

        // Assert
        Assert.IsTrue(callbackCalled);
    }

    [TestMethod]
    [TestCategory("Runtime.Game - NotificationManager")]
    public void Handler_can_unsubscribe()
    {
        var notificationCenter = new NotificationManager();
        int callCount = 0;

        // Build our notification.
        ISubscription subscriber = notificationCenter.Subscribe<MessageFixture>(
            (message, sub) => callCount++);

        // Subscribe our notification and publish a new message
        notificationCenter.Publish(new MessageFixture("Test"));

        // Act
        // Unsubscribe the notification and attempt a new publish
        subscriber.Unsubscribe();
        notificationCenter.Publish(new MessageFixture("Test"));

        // Assert
        Assert.AreEqual(1, callCount, "The callbacks were not fired properly");
    }

    [TestMethod]
    [TestCategory("Runtime.Game - NotificationManager")]
    public void Handler_receives_only_its_message()
    {
        // Set up the first handler
        var notificationCenter = new NotificationManager();
        notificationCenter.Subscribe<MessageFixture>(
            (message, sub) => 
                ExceptionFactory.ThrowIf<InvalidOperationException>(message.GetType() != typeof(MessageFixture)));
        notificationCenter.Subscribe<SecondaryMessageFixture>(
            (message, sub) => 
                ExceptionFactory.ThrowIf<InvalidOperationException>(message.GetType() != typeof(SecondaryMessageFixture)));

        // Act
        notificationCenter.Publish(new MessageFixture("Test"));
        notificationCenter.Publish(new SecondaryMessageFixture(new GameComponentFixture()));
    }
}

I plan on using the NotificationManager not just for chat messages, but game event messages that will get logged to the server console. I plan on building commanding around this, so a user enters a command that gets published, and a subscribing CommandManager will receive the message and run it through a TPL data flow to process the command in to an IPlayerCommand that maps to the supplied command and executed.

Do you guys feel my NotificationManager will be responsible for to much and I should break it in to different managers per message catagory (events, chat, commanding) or is it acceptable to run them all through the NotificationManager?

My though being that I would build different INotification implementations so that the messages published can be processed differently. Instead of a shared Notification class, I'd have ChatNotification and CommandNotification, with each handling how the notification message is processed differently (example being CommandNotification firing a TPL Dataflow).

I felt that having the NotificationManager just hand-off the publications to each INotification instance and not really holding any logic, would keep with SRP and allow several notification categories to be used with the same object.

I'd appreciate your input

\$\endgroup\$
3
\$\begingroup\$
object GetContent();
TContent Content { get; }
new TContent GetContent();

How is GetContent() (both versions) useful? The non-generic IMessage could be just a marker interface, with no methods.

I'm not even sure about the usefulness of Content, since it seems it doesn't contain all the content of the message. E.g. treating WhisperMessage as a IMessage<string> won't give you everything.


if (this.condition == null)
{
    this.callback(message, this);
    return;
}

if (!this.condition(message))
{
    return;
}

this.callback(message, this);

This is unnecessarily complicated, you could simplify it to:

if (this.condition == null || this.condition(message))
{
    this.callback(message, this);
}

List<ISubscription> subscribers = listeners[messageType];
lock (listenerCollectionLock)
{
    subscribers.Add(handler);
}

You could lock here (and in Unsubscribe()) on subscribers. That way, adding subscribers for two different message types could happen at the same time.

\$\endgroup\$
  • \$\begingroup\$ Nice catch on locking on the local subscribers. \$\endgroup\$ – Johnathon Sullinger Jan 19 '15 at 16:47
2
\$\begingroup\$

IMessage<TContent>

Here you have a property TContentType Content and also a TContentType GetContent() method, which are just returning the same. If you assume that returning the content will be fast (there won't be time consuming calculation involved) you should make it a property otherwise a method. Having both is a code smell (at least for me).

Notification<TMessage>

The ProcessMessage() message could be rewritten to combine the check for condition == null and !this.condition(message) to remove the small code duplication.

public void ProcessMessage(TMessage message)
{

    if (this.condition != null && !this.condition(message))
    {
        return;
    }

    this.callback(message, this);
}   

NotificationManager

In the Subscribe<TMessageType>() method you should remove the assignment of the dictionary value to a subscribers variable because you don't use it afterwards.

lock (listenerCollectionLock)
{
    listeners[messageType].Add(handler);
}  

The same is true for the Unsubscribe() method

lock (listenerCollectionLock)
{
    listeners[args.MessageType].Remove(args.Subscription);
}

General

  • If the primary purpose is to send String messages I wouldn't go the generic route.

  • Using XML documentation is good, but it should be complete.
    For the INotificationCenter.SubScribe() method you are missing the documentation for the condition.

  • I appreciate the using of braces {} for single if statements

  • I appreciate the using of guard clauses/conditions to save horizontal space
\$\endgroup\$
  • \$\begingroup\$ The subscribers collection has been removed. It was left over from when the collection was a ConcurrentBag and I had to do TryGet(out subscribers). I will go back over my comments and make sure their fully documented. \$\endgroup\$ – Johnathon Sullinger Jan 19 '15 at 16:42
  • \$\begingroup\$ I like your point on the TContentType GetContent(). It doesn't do anything other than just return the TContentType property. I'll remove the method from the generic interface and just use the property. \$\endgroup\$ – Johnathon Sullinger Jan 19 '15 at 16:43

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.