9
\$\begingroup\$

I'm developing a mediator for an MVVM application. This application lets employees file tickets with IT support to ask for help. The mediator communicates messages and triggers callbacks between ViewModels. Here's a description of it's architecture :

The messages between the ViewModels

The messages are stored in an enumerator :

public enum ViewModelMessages
{
    UserCreated,
    ManagerChanged,
    TicketCreated,
    TicketDeleted,
    TicketRefusedByIT,
    CommentAppendedToTicket,
    TicketCompleted,
    StatisticsFileOutputted
};

This list of messages could get a lot longer than that as I continue development.

The Mediator

The mediator is a singleton that can register multiple callbacks for each message and triggers them when the message is emitted. These callbacks can take no parameters or a single object parameter of any given type :

public sealed class Mediator {
    #region Singleton definition
    private static readonly Lazy<Mediator> lazy = new Lazy<Mediator>(() => new Mediator());

    public static Mediator Instance {
        get {
            return lazy.Value;
        }
    }

    private Mediator() {
    }
    #endregion

    private MultiDictionary<ViewModelMessages, Action> registeredCallbacksWithoutParameter = new MultiDictionary<ViewModelMessages, Action>();
    private MultiDictionary<ViewModelMessages, Action<object>> registeredCallbacksWithParameter = new MultiDictionary<ViewModelMessages, Action<object>>();

    public void Register(ViewModelMessages message, Action callback) {
        registeredCallbacksWithoutParameter[message] += callback;
    }

    public void Register<ParameterType>(ViewModelMessages message, Action<ParameterType> callback) {
        registeredCallbacksWithParameter[message] +=
            (parameter) => callback((ParameterType)parameter);
    }

    public void Unregister(ViewModelMessages message, Action callback) {
        registeredCallbacksWithoutParameter[message] -= callback;
    }

    public void Unregister(ViewModelMessages message, Action<object> callback) {
        registeredCallbacksWithParameter[message] -= callback;
    }

    public void Notify(ViewModelMessages message) {
        foreach(Action callback in registeredCallbacksWithoutParameter[message]) {
            callback();
        }
    }

    public void Notify(ViewModelMessages message, object parameter) {
        foreach(Action callback in registeredCallbacksWithoutParameter[message]) {
            callback();
        }
        foreach(Action<object> callback in registeredCallbacksWithParameter[message]) {
            callback(parameter);
        }
    }
}

The MultiDictionary

The callbacks are stored in a MultiDictionary: a regular dictionary where each entry can store a hashset :

public class MultiDictionary<keyType, contentType> : IDictionary<keyType, AddableHashSet<contentType>> {
    private Dictionary<keyType, AddableHashSet<contentType>> dictionary = new Dictionary<keyType, AddableHashSet<contentType>>();

    public AddableHashSet<contentType> this[keyType key] {
        get {
            EnsureKey(key);
            return dictionary[key];
        }
        set {
            EnsureKey(key);
            dictionary[key] = value;
            if(dictionary[key].Count == 0) {
                dictionary.Remove(key);
            }
        }
    }

    private void EnsureKey(keyType key) {
        if(!ContainsKey(key)) {
            dictionary.Add(key, new AddableHashSet<contentType>());
        }
        else {
            if(dictionary[key] == null)
                dictionary[key] = new AddableHashSet<contentType>();
        }
    }

    #region IDictionary interface implementation via the dictionnary member variable
    public ICollection<keyType> Keys => dictionary.Keys;
    public ICollection<AddableHashSet<contentType>> Values => dictionary.Values;
    public int Count => dictionary.Count;
    public bool IsReadOnly => ((IDictionary<keyType, AddableHashSet<contentType>>)dictionary).IsReadOnly;
    public void Add(keyType key, AddableHashSet<contentType> value) => dictionary.Add(key, value);
    public void Add(KeyValuePair<keyType, AddableHashSet<contentType>> item) => throw new NotImplementedException();
    public void Clear() => dictionary.Clear();
    public bool Contains(KeyValuePair<keyType, AddableHashSet<contentType>> item) => throw new NotImplementedException();
    public bool ContainsKey(keyType key) => dictionary.ContainsKey(key);
    public void CopyTo(KeyValuePair<keyType, AddableHashSet<contentType>>[] array, int arrayIndex) => throw new NotImplementedException();
    public IEnumerator<KeyValuePair<keyType, AddableHashSet<contentType>>> GetEnumerator() => dictionary.GetEnumerator();
    public bool Remove(keyType key) => dictionary.Remove(key);
    public bool Remove(KeyValuePair<keyType, AddableHashSet<contentType>> item) => throw new NotImplementedException();
    public bool TryGetValue(keyType key, out AddableHashSet<contentType> value) => dictionary.TryGetValue(key, out value);
    IEnumerator IEnumerable.GetEnumerator() => throw new NotImplementedException();
    #endregion
}

The number of callbacks per message will potentially be between 0 and 10, but it could potentially go a lot higher than that. At this stage I still don't know how high so I preferred to use a hashet instead of a list to avoid it being a bottleneck later on.

The hashset

The AddableHashSet is a hashset with the + and - operators overloaded to simplify the syntax in the Mediator: using += and -= follows the event += eventhandler() pattern.

public class AddableHashSet<Type> : HashSet<Type> {

    public static AddableHashSet<Type> operator +(AddableHashSet<Type> hashSet, Type value) {
        hashSet.Add(value);
        return hashSet;
    }

    public static AddableHashSet<Type> operator -(AddableHashSet<Type> list, Type value) {
        if(list.Contains(value)) {
            list.Remove(value);
        }
        return list;
    }

    public AddableHashSet() {
    }
}

What do you think?

This is my first implementation of the mediator pattern ever. Any suggestions would be greatly appreciated!

\$\endgroup\$
  • 2
    \$\begingroup\$ much better than your old question :) Do note: it's possible to edit your question. This is especially true after the question has been closed. Editing a closed question will put it into the reopen queue. Just for future reference \$\endgroup\$ – Vogel612 Jul 28 '17 at 12:43
  • \$\begingroup\$ Have you looked into the Reactive Extension framework for this? Or was there a reason you couldn't use that? The syntax is different but it can accomplish the same thing and give even more flexibility. \$\endgroup\$ – CharlesNRice Jul 28 '17 at 14:08
  • \$\begingroup\$ I can't use a framework because the future maintainers already have to learn C#, WPF and OOP and don't want the overhead of a framework on top of that. This is also for academic purposes. \$\endgroup\$ – Queder Jul 28 '17 at 14:55
  • \$\begingroup\$ I would totally use Mediatr instead of rolling my own implementation. \$\endgroup\$ – Hosch250 Jul 29 '17 at 16:46
7
\$\begingroup\$

I don't feel like you should override + and -. Fine, it makes the syntax cuter, but it makes your code much less readable for someone who isn't you.

Remember, you write code so that yourself in 5 years or someone else can read it properly, not so you can feel it better right now. Follow this guideline, and you'll see that your code will be much cleaner.

By using the + operator, we loose the advantage of the bool Add(T value) that the Hashset<T> has.

Following coding conventions, your generic types should be PascalCased. So keyType = KeyType and contentType = ContentType.

You could make your dictionary variable readonly. The main purpose is to tell other programmers (or yourself in 5 years) that this variable should never be set somewhere else than at it's declaration.

I'm wondering, why is there a s at the end of ViewModelMessages? Is it a list of view models?

Your Mediator looks fine, though I feel like there might be a problem when it comes to Unregister. Imagine this :

Mediator.Instance.Register(ViewModelMessages.UserCreated,() => someAction);

//Later in a far far away block of code

Mediator.Instance.Unregister(ViewModelMessages.UserCreated, () => someAction);

Will it work? After all, the lambda isn't the same reference as before! Your dictionary might not find it. (Although I must say I'm not 100% sure)

This is why, when you look at the IObservable/IObserver pattern, IObservable<T>.Subscribe returns IDisposable, which is the object responsible to Unsubscribe the Observer, you should implement something similar in my opinion because you might not always have the opportunity to Unregister your callback when you want (example if you need to do it in a DLL that has no reference to your Mediator.

Last thing, I don't think it should be a Singleton. I think your Mediator should be backed by an interface (IMediator perhaps) so you can unit test your code more easily and create less dependance around your code. (If you need to understand why interfaces are nice, I'll post some articles in comment if you want) And you could use an IoC container to deal with the dependances.

\$\endgroup\$
  • \$\begingroup\$ I add an "s" at the end of my enums to represent the fact that they contain multiple values of something. Here each value represents a message that can be sent by a ViewModel, so my enum is ViewModelMessages . I would gladly read any article you could provide. Thanks a lot. \$\endgroup\$ – Queder Jul 28 '17 at 15:20
  • 1
    \$\begingroup\$ @Queder I don't think this is part of the naming conventions, I think enum should be singular! I'll post links to articles soon \$\endgroup\$ – IEatBagels Jul 28 '17 at 18:14
  • 4
    \$\begingroup\$ Actually it is part of the naming convention: msdn.microsoft.com/de-de/library/4x252001(v=vs.71).aspx ;) \$\endgroup\$ – JanDotNet Jul 28 '17 at 18:28
  • \$\begingroup\$ @JanDotNet It says it should be pluralized if the enum is a bit enum, which doesn't seem to be the case here. If so, the enum lacks the Flag attribute \$\endgroup\$ – IEatBagels Jul 30 '17 at 1:49
  • 2
    \$\begingroup\$ @TopinFrassi: Exactly, it lacks the Flags attribute or it's name should be singular as suggested by you. :) \$\endgroup\$ – JanDotNet Jul 30 '17 at 5:33
5
\$\begingroup\$

You should not be calling it a dictionary because you changed the expected behaviour of the IDictionary interface. The MSDN clearly states that the getter of the indexer should throw an exception if the key is not found.

KeyNotFoundException - The property is retrieved and key is not found.

this should not be adding new elements if the getter is used. The closest collection type that has a similar behaviour would be the ILookup but even this doesn't seem to be right in this case - a lookup returns an empty collection but does not change the lookup itself.

I suggest naming it a MultiCollection and documenting the unexpected side effects of the getter. This collection does not need to implement the IDictionary interface because you use only the indexer anyway. IEnumerable would be enough so that you can use it a loop.


You are also doing other things that are very unusual for C#.

For example you use a plural name for the enum. We do this only if it is also decorated with the FlagsAttribute that indicates that the values can be combined.

Another strange naming convention is the <keyType, contentType>. C# uses a T prefix for generic types so the correct names should be: <TKey, TContent>.

\$\endgroup\$
5
\$\begingroup\$

Don't test whether a HashSet contains a key if you want to remove it:

if(list.Contains(value)) {
    list.Remove(value);
}

Instead, just straight try to remove it:

list.Remove(value);

The replacement is cleaner, shorter, and more efficient. What's not to like?

\$\endgroup\$
5
\$\begingroup\$

I fully agree with TopinFrassi and t3chb0t.

Some other points

  • The MultiDictionary and the AddableHashSet are not required and complicate the solution. It is much simpler to use a dictionary with a List<TValue> as value. I understand the desire for simplifying the API to accessing the dictionary, but that can be also realized using extension methods or by sub-classing the dictionary.
  • Using an enum as key may become a problem because the enum contains business related code (or values) and the Mediator is more a framework component - these parts should IMHO not be mixed. You could resolve that issue by using generics, or by using a more general type as key (e.g. object or string).
  • The Lazy type is IMHO not required for singletons because it makes the code less readable and have no benefit.

Considering the points above may result in the following implementaion:

public sealed class Mediator<TMessageKey>
{
    public static Mediator<TMessageKey> Instance { get; } = new Mediator<TMessageKey>();

    private Mediator() { }

    private readonly KeyListDictionary<TMessageKey, Action> callbacks
        = new KeyListDictionary<TMessageKey, Action>();
    private readonly KeyListDictionary<TMessageKey, Action<object>> callbacksWithParameter
        = new KeyListDictionary<TMessageKey, Action<object>>();

    public void Register(TMessageKey key, Action action)
        => callbacks.AddListEntry(key, action);
    public void Register<ParameterType>(TMessageKey key, Action<object> action)
        => callbacksWithParameter.AddListEntry(key, action);

    public void Unregister(TMessageKey key, Action action)
        => callbacks.RemoveListEntry(key, action);
    public void Unregister(TMessageKey key, Action<object> action)
        => callbacksWithParameter.RemoveListEntry(key, action);

    public void Notify(TMessageKey key)
        => callbacks.ForEachListEntry(key, action => action());
    public void Notify(TMessageKey key, object parameter)
        => callbacksWithParameter.ForEachListEntry(key, action => action(null));
}

public class KeyListDictionary<TKey, TListEntry> : Dictionary<TKey, IList<TListEntry>>
{
    public void AddListEntry(TKey key, TListEntry value)
    {
        IList<TListEntry> list;
        if (!TryGetValue(key, out list))
        {
            list = new List<TListEntry>();
            Add(key, list);
        }
        list.Add(value);
    }

    public bool RemoveListEntry(TKey key, TListEntry value)
    {
        IList<TListEntry> list;
        if (TryGetValue(key, out list))
            return list.Remove(value);
        return false;
    }

    public void ForEachListEntry(TKey key, Action<TListEntry> action)
    {
        IList<TListEntry> list;
        if (TryGetValue(key, out list))
            foreach (var value in list)
                action(value);
    }
}
\$\endgroup\$
  • \$\begingroup\$ The extension methods make the code less readable though: my first knee-jerk reaction was "This code is obviously wrong" before I scrolled down to the MultiDictionnaryHelper. I prefer to use a custom class which encapsulates the desired behavior because it's more traceable. However, separating concerns by using a generic type for the message key is a good point., and the Lazy does make the code less readable. \$\endgroup\$ – Queder Aug 1 '17 at 8:29
  • 1
    \$\begingroup\$ my first knee-jerk reaction was "This code is obviously wrong": Good point - may be valid from the view of a reader... Probably it is better to rename the extension methods to distinguish it from the other Add/Remove methods of the dictionary. Maybe it is even better (more readable?) when sub-classing the dictionary instead of using extension methods... pls see my updated solution. \$\endgroup\$ – JanDotNet Aug 1 '17 at 8:44
  • 1
    \$\begingroup\$ Generally I think, that extension methods are more appropriate and easier to understand than a custom class that overwrites operators in an unusual manner. \$\endgroup\$ – JanDotNet Aug 1 '17 at 8:53

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.