0
\$\begingroup\$

I'm implementing a Mediator and I'd like to simplify the callback notation a bit.

My mediator has Register and Notify methods :

public class Mediator {
    private Dictionary<Messages, Action<object>> registeredCallback;

    public void Register(Messages message, Action callback) {
        registeredCallback[message] = callback;
    }

    public void Notify(Messages message, object parameter) {
        callback(parameter);
    }

    public Mediator() {
        registeredCallback = new Dictionary<Messages, Action<object>>();
    }
}

It uses a Messages enumerator to sanitize events between registered objects, for example :

public enum Messages {
    UserCreated,
    UserDeleted,
    UserChangedPassword
};

So far, so good. However, when I use this mediator I'm making ugly callback definitions :

mediator.Register(Messages.UserCreated, (object user) => GreetUser(((User)user).name));

with the following User:

public class User {
    public string name;
}

Type casting? Lambda expressions? This looks obviously filthy. How can I clean this up? Is my architecture at fault?

\$\endgroup\$
9
  • \$\begingroup\$ I think it's the first time I see someone complain that Lambda are filthy :P Honestly I think your code is good. Sometimes, type casting is necessary. \$\endgroup\$ Commented Jul 27, 2017 at 16:02
  • 2
    \$\begingroup\$ I find lambdas filthy because they don't have a name. They don't explain what they do, they aren't reusable... I think that's confusing in real-life situations? Thank you for the compliment, though! \$\endgroup\$ Commented Jul 27, 2017 at 16:06
  • \$\begingroup\$ I'm sorry but this is not real C# and the code is broken. You use the registeredCallbacks[message] in a loop but the value is an Action<object> and it cannot be enumerated. \$\endgroup\$ Commented Jul 27, 2017 at 16:35
  • \$\begingroup\$ @t3chb0t I'm actually using a custom MultiDictionary which stores multiple callbacks in a HashSet<Action<object>> for each message. I didn't include it in my example to simplify a bit. I removed the loop, sorry for the confusion. \$\endgroup\$ Commented Jul 27, 2017 at 16:49
  • 2
    \$\begingroup\$ I didn't include it in my example to simplify a bit. This is usually a mistake becasue if your code isn't real then what is the point of reviewing it? People will sugest improvements that you either already have implemented or are irrelevant. It doesn't help anyone. \$\endgroup\$ Commented Jul 27, 2017 at 16:53

2 Answers 2

Reset to default
3
\$\begingroup\$

What about making the Register method generic?

public class Mediator
{
    private Dictionary<Messages, Action<object>> registeredCallback =
        new Dictionary<Messages, Action<object>>();

    public void Register<TParam>(Messages message, Action<TParam> callback)
    {
        registeredCallback[message] = p => callback((TParam)p);
    }

    public void Notify(Messages message, object parameter)
    {
        callback(parameter);
    }
}

The typecasting is done inside the Register method, making it invisible to any class using it.

You can also change signature of the GreetUser method so that it takes a User instead of a string as a parameter. By doing so you can call Register like this:

mediator.Register<User>(Messages.UserCreated, GreetUser);
\$\endgroup\$
1
  • \$\begingroup\$ Using a generic Register method is the best solution for this. It works perfectly with a method taking the same object type as argument. Thank you so much for the input! \$\endgroup\$ Commented Jul 27, 2017 at 17:41
1
\$\begingroup\$

Since your comment said you didn't like lambdas, let me propose something!

You could use an interface, let's say IMediatorCallBack. Then, you would need to create a class for each callback. It may look like more trouble than it's worth, but if you have a lot of callbacks, it might be worth it.

public interface IMediatorCallBack 
{
    void Execute(object obj);
}

internal class UserCreatedMediatorCallBack : IMediatorCallBack
{
    public void Execute(object obj)
    {
        //At this point, you know that the callback is called by the mediator, so just cast it.
        var user = (User)obj;
        //etc..
    }
}

Notice that the class is internal. I think that's the way to go since it's pretty weird that we cast the object to User right away.

Then you could use your Dictionary<Messages,IMediatorCallBack>, which "talks" a little bit more.

I'm not 100% sure it's a better solution, but you wanted something without lambdas, there it is :)

\$\endgroup\$
1
  • \$\begingroup\$ I don't have a lot of callbacks, but I do have a lot of messages in my enum, so implementing a callback class for each message only to have it used once or twice is a bit too much. +1 for the interface though, I didn't think of that and it might be of use later. \$\endgroup\$ Commented Jul 27, 2017 at 16:52

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.