HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Mediator pattern implementation for game messaging

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
messaginggameformediatorimplementationpattern

Problem

I am building out a MUD game engine and want my objects to communicate with each other. There is a wide range of message types that will be sent around, all of which implement IMessage.

I adopted the Mediator pattern to facilitate passing the messages around to objects that have subscribed for publications. Is the pattern being used properly here?

INotificationCenter and implementation

The interface represents what objects will interact with when they want to subscribe to publications for a specific IMessage type. The ChatCenter is implemented as a singleton and will ultimately end up with the Dictionary being changed to a ConcurrentDictionary for thread-safety.

```
public interface INotificationCenter
{
ISubscriptionHandler Subscribe() where T : class, IMessage;

void Publish(T message) where T : class, IMessage;
}

///
/// The mediator for all messaging
///
public class ChatCenter : INotificationCenter
{
///
/// Collection of subscribed listeners
///
private Dictionary> listeners =
new Dictionary>();

private static ChatCenter _centerSingleton = new ChatCenter();

private ChatCenter()
{
}

///
/// Subscribe publications for the message type specified.
///
///
///
public ISubscriptionHandler Subscribe() where T : class, IMessage
{
Type messageType = typeof(T);

if (!listeners.ContainsKey(messageType))
{
listeners.Add(messageType, new List());
}

// TODO: Move instancing of the handler in to a factory that does a lookup on and returns the right handler.
var handler = new ChatMessageHandler();
listeners[messageType].Add(handler);

return handler;
}

public static ChatCenter CurrentCenter
{
get
{
return _centerSingleton;
}
}

///
/// Publishes the specified message to all subscribers
///
///
/// The message.
public void P

Solution

ISubscriptionHandler Subscribe()


I think this is a confusing interface. I would expect something along the lines of:

ISubscription Subscribe(ISubscriptionHandler handler)


where ISubscriptionHandler would contain any predicates and code that should be dispatched when a message arrives and ISubscription can be used to unsubscribe.

Why is ChatCenter a singleton? This is especially weird since you're using unit tests, which should be independent.

/// 
/// 


If you don't want to add documentation comments for everything, that's fine. But there is no reason to have empty comments like these cluttering up your code.

You already realized that you need to separate the non-generic ISubscriptionHandler and the generic ISubscriptionProcessor. But since you already have that, you should use the generic version as much as possible and use the non-generic version only when necessary (pretty much just around the Dictionary).

This would mean you could get rid of the IMessage.Dispatch() step and for example the core of ChatCenter.Publish() could look like this (explicitly typing the iteration variable of a foreach works as a cast):

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


public interface ISubscriptionHandler
{
    ISubscriptionHandler If(Func condition);

    ISubscriptionHandler Dispatch(Action message);

    void Unsubscribe();
}


Even if you made this type generic as I suggested above, this interface still feels clunky to me. Why do you even need multiple conditions and multiple handler delegates? And if you think that having single condition is not enough, why would multiple conditions joined by AND be enough?

Just allow only a single condition and if the user wants something more complicated, they can create that complicated condition themselves.

Action message


This delegate is not a message, so it shouldn't be called that. Maybe something like messageProcessor?

public interface ISubscriptionProcessor


Generic parameter can't be anything else than a type, so you need to spell it out, just TMessage is enough. (Also “The type of the message type.” is confusing.)

public class ChatMessageHandler : ISubscriptionProcessor


This whole type doesn't contain anything that's specific to ChatMessage. Instead, it should be a generic type like:

public class MessageHandler : ISubscriptionProcessor


var msg = this as TMessageType;
if (msg == null)
{
    return;
}


This situation would mean that the current type of message is broken (it has incorrect type parameter). I think the right behavior in that case is to complain loudly by throwing an exception, not silently doing nothing.

This way, the programmer who wrote the faulty message type will realize their mistake very soon, they won't have to spend time figuring out why their message doesn't seem to do anything.

Very similar argument applies to the following handler check.

Code Snippets

ISubscriptionHandler Subscribe<T>()
ISubscription Subscribe<T>(ISubscriptionHandler<T> handler)
/// <typeparam name="T"></typeparam>
/// <returns></returns>
foreach (ISubscriptionProcessor<T> handler in listeners[typeof(T)])
{
    handler.ProcessMessage(message);
}
public interface ISubscriptionHandler
{
    ISubscriptionHandler If(Func<IMessage, bool> condition);

    ISubscriptionHandler Dispatch(Action<IMessage> message);

    void Unsubscribe();
}

Context

StackExchange Code Review Q#77051, answer score: 3

Revisions (0)

No revisions yet.