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

Messenger supporting notifications and requests

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

Problem

I've written a lightweight (I think) class that acts as a messenger service between classes for both notifications (fire and forget updates to other classes) and requests (a notification sent out that expects a returned value).

I'm looking for a general review here on style, usability, best practices, etc.

Here's the code (.NET Fiddle with usage):

```
public class Messenger
{
///
/// Gets the default messenger.
///
/// The default messenger.
public static Messenger Default
{
get
{
defaultMessenger = defaultMessenger ?? new Messenger();

return defaultMessenger;
}
}

///
/// The default messenger
///
private static Messenger defaultMessenger;

///
/// The actions hooked up to messages
///
private Dictionary actions = new Dictionary();

///
/// The functions hooked up to requests
///
private Dictionary functions = new Dictionary();

///
/// Register a function for a request message.
///
/// Type of message to receive.
/// Type of the r.
/// The function that fills the request.
public void Register(Func request)
{
if (request == null)
{
throw new ArgumentNullException("request");
}

if (functions.ContainsKey(typeof(T)))
{
functions[typeof(T)] = Delegate.Combine(functions[typeof(T)], request);
}
else
{
functions.Add(typeof(T), request);
}
}

///
/// Register an action for a message.
///
/// Type of message to receive.
/// The action that happens when the message is received.
public void Register(Action action)
{
if (action == null)
{
throw new ArgumentNullException("action");
}

if (actions.ContainsKey(typeof(T)))
{
actions[typeof(T)] = (Action)Delegate.Combine(actions[typeof(T)], action);

Solution

-
Given that Messenger is a fairly light weight class I don't quite see the point of lazy initialization for the static instance. Why not simply use a static initializer and a readonly field

public class Messenger
{
     public static readonly Messenger Default = new Messenger();
}


-
I would get rid of the Default static instance. It tends to encourage bad practices. It's easy enough to add later when there is a use case where it really can't be solved any other way but having it there just for convenience purposes is a bad idea.

-
The cast here:

actions[typeof(T)] = (Action)Delegate.Combine(actions[typeof(T)], action);


is unnecessary since actions uses Delegate as value type.

-
I'm somewhat dubious about the usefulness being able to register multiple Funcs in an invocation list. The result of calling that invocation list will be the result of the last method being called hence the result will depend on the order of registration which is generally brittle to rely on.

Code Snippets

public class Messenger
{
     public static readonly Messenger Default = new Messenger();
}
actions[typeof(T)] = (Action<T>)Delegate.Combine(actions[typeof(T)], action);

Context

StackExchange Code Review Q#71015, answer score: 4

Revisions (0)

No revisions yet.