5
\$\begingroup\$

I have extended this class http://wiki.unity3d.com/index.php/CSharpMessenger_Extended to accept strings containing wildcards as eventTypes, therefore I am able to write things like

Messenger.AddListener("*", () => Console.WriteLine("woohoo, generic handler"));

But I am highly concerned about the performance of this implementation.

I've extended the Dictionary class like this:

public class EventDictionary : Dictionary<string, Delegate>
    {
        public Delegate[] GetHandlers(string Event)
        {
            List<Delegate> matches = new List<Delegate>();
            foreach (string pattern in Keys)
                if (Regex.IsMatch(Event, Regex.Escape(pattern).Replace( @"\*", ".*" ).Replace( @"\?", "." )))
                    matches.Add(this[pattern]);
            UnityEngine.Debug.Log(matches.Count);
            return matches.ToArray();
        }
    }

As you see, for every key in the dictionary, I first convert it to a regular expression, then check if it is a match for the event name, if it's so, add it to a collection and return the array.

But, this code runs in a game where the messenger script is used frequently, therefore I am really concerned about the performance of this approach.

I think storing regex's directly in the dictionary would be better but my main concern is about the regex matching.

Is my solution acceptable? If not what would be a better approach for solving my problem?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! That is an interesting situation, I hope you get some good reviews! \$\endgroup\$ – Phrancis Dec 3 '14 at 20:58
4
\$\begingroup\$

Here are some problems I see with your code:

  • The code removes the Dictionary's constant-time lookup.
  • The code reparses the pattern and recompiles each regex every time you're in the loop.
  • The code uses regex's even for simple lookups.
  • The code will die if you try to assign more than one handler to an event.
  • This might just be an artifact of you copying/pasting, but the indenting on your method is a little unusual.
  • The code does not favor composition over inheritance.
  • Your parameter name does not follow the convention of being camelCase.
  • Your regex pattern doesn't require the beginning and ends, so e.g. as?f matches the same things as *as?f*.

I would use the built-in Dictionary lookup functionality for simple (no-wildcard) lookups and store compiled regex's for the ones with wildcards. Also, you can look at the reference source in your link to get an idea of how to solve the problem of multiple handlers for an event.

This should be a good starting point, with code to add a listener and get handlers. It only uses regex's when needed, keeping them precompiled in a second Dictionary. If the "*" case is pretty common, you could further improve this by handling that as a special case: you don't really need to compile and store a Regex for it, you just always run it. This would probably be a third field, maybe private Delegate universalHandlers;

public class EventDictionary
{
    private readonly Dictionary<string, Delegate> eventTable = new Dictionary<string, Delegate>();
    private readonly Dictionary<string, Regex> regexTable = new Dictionary<string, Regex>();

    private bool HasWildcards(string pattern)
    {
        return pattern.Contains("*") || pattern.Contains("?");
    }

    public void AddListener(string eventType, Delegate eventDelegate)
    {
        Delegate currentDelegate;
        if (eventTable.TryGetValue(eventType, out currentDelegate))
        {
            var d = eventTable[eventType];
            if (d != null && d.GetType() != eventDelegate.GetType())
                throw new ArgumentException(string.Format("Attempting to add listener with inconsistent signature for event type {0}. Current listeners have type {1} and listener being added has type {2}", eventType, d.GetType().Name, eventType.GetType().Name));
            eventTable[eventType] = Delegate.Combine(currentDelegate, eventDelegate);
        }
        else
        {
            eventTable.Add(eventType, eventDelegate);
        }

        if (HasWildcards(eventType) && !regexTable.ContainsKey(eventType))
            regexTable.Add(eventType, new Regex("^" + Regex.Escape(eventType).Replace(@"\*", ".*").Replace(@"\?", ".") + "$"));
    }

    // TODO implement remove method

    public Delegate GetHandlers(string eventType)
    {
        Delegate handler;
        eventTable.TryGetValue(eventType, out handler);
        foreach (var pair in regexTable)
            if (pair.Value.IsMatch(eventType))
                handler = Delegate.Combine(handler, eventTable[pair.Key]);
        UnityEngine.Debug.Log(handler.GetInvocationList().Length);
        return handler;
    }
}
\$\endgroup\$
  • \$\begingroup\$ This is pretty good and comprehensive. I understand everything but why my code was going to die if I attached 2 listeners to an event. Indentation is due to copy/paste you are right. \$\endgroup\$ – Fatih BAKIR Dec 4 '14 at 6:49
  • \$\begingroup\$ @FatihBAKIR As I understand it, you were going to attach listeners like eventDictionary.Add("someEvent", myDelegate). A Dictionary<,> can only hold one value for each key, so this will throw an error on the 2nd listener. If you were just swapping out the Dictionary class used in MessengerInternal with your class, then you were fine in that regard. My example basically replaces the whole Messenger class. You can choose how to best piece these things together. \$\endgroup\$ – Tim S. Dec 4 '14 at 12:41

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.