5
\$\begingroup\$

Given the following event dispatcher code

public interface IApplicationEvent { }

public delegate void ApplicationEventHandlerDelegate<in TEvent>(TEvent @event) where TEvent : IApplicationEvent;

public class ApplicationEventDispatcher
{
    private bool _disposed;
    private Dictionary<Type, Delegate> _applicationEventHandlers;

    public ApplicationEventDispatcher()
    {
        _applicationEventHandlers = new Dictionary<Type, Delegate>();
    }

    ~ApplicationEventDispatcher()
    {
        Dispose(false);
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    private void Dispose(bool disposing)
    {
        if (_disposed) return;

        if (disposing)
        {
            // free other managed objects that implement IDisposable only
            RemoveAllListeners();
        }

        // release any unmanaged objects
        // set the object references to null
        _applicationEventHandlers = null;

        _disposed = true;
    }

    public void AddListener<TEvent>(ApplicationEventHandlerDelegate<TEvent> handler)
        where TEvent : IApplicationEvent
    {
        Delegate @delegate;
        if (_applicationEventHandlers.TryGetValue(typeof(TEvent), out @delegate))
        {
            _applicationEventHandlers[typeof(TEvent)] = Delegate.Combine(@delegate, handler);
        }
        else
        {
            _applicationEventHandlers[typeof(TEvent)] = handler;
        }
    }

    public void RemoveListener<TEvent>(ApplicationEventHandlerDelegate<TEvent> handler)
        where TEvent : IApplicationEvent
    {
        Delegate @delegate;
        if (_applicationEventHandlers.TryGetValue(typeof(TEvent), out @delegate))
        {
            Delegate currentDel = Delegate.Remove(@delegate, handler);

            if (currentDel == null)
            {
                _applicationEventHandlers.Remove(typeof(TEvent));
            }
            else
            {
                _applicationEventHandlers[typeof(TEvent)] = currentDel;
            }
        }
    }

    public void Dispatch<TEvent>(TEvent @event) where TEvent : IApplicationEvent
    {
        if (@event == null) throw new ArgumentNullException("event");
        if (_disposed) throw new ObjectDisposedException("Cannot dispatch and event when disposed! ");

        Delegate @delegate;
        if (_applicationEventHandlers.TryGetValue(typeof(TEvent), out @delegate))
        {
            ApplicationEventHandlerDelegate<TEvent> callback = @delegate as ApplicationEventHandlerDelegate<TEvent>;
            if (callback != null)
            {
                callback(@event);
            }
        }
    }

    private void RemoveAllListeners()
    {
        var handlerTypes = new Type[_applicationEventHandlers.Keys.Count];
        _applicationEventHandlers.Keys.CopyTo(handlerTypes, 0);

        foreach (Type handlerType in handlerTypes)
        {
            Delegate[] delegates = _applicationEventHandlers[handlerType].GetInvocationList();
            foreach (Delegate @delegate1 in delegates)
            {
                var handlerToRemove = Delegate.Remove(_applicationEventHandlers[handlerType], @delegate1);
                if (handlerToRemove == null)
                {
                    _applicationEventHandlers.Remove(handlerType);
                }
                else
                {
                    _applicationEventHandlers[handlerType] = handlerToRemove;
                }
            }
        }
    }
}

Is there a better way than this to check if an event handler has been called, or not called correctly?

    [TestMethod]
    public void GivenAnEventToDispatch_WhenAHandlerIsAttached_CallsHandler()
    {
        // ARRANGE
        bool handlerCalled = false;
        ApplicationEventHandlerDelegate<SimpleEvent1> @delegate = 
            delegate { 
                handlerCalled = true; 
            };
        var dispatcher = new ApplicationEventDispatcher();
        dispatcher.AddListener(@delegate);

        // ACT
        dispatcher.Dispatch(new SimpleEvent1());
        dispatcher.Dispose();

        // ASSERT
        Assert.IsTrue(handlerCalled);
    }

    [TestMethod]
    public void GivenAnEventToDispatch_WhenAHandlerIsAttachedAndDetached_DoesNotCallHandler()
    {
        // ARRANGE
        bool handlerCalled = false;
        ApplicationEventHandlerDelegate<SimpleEvent1> @delegate =
            delegate {
                handlerCalled = true; 
            };
        var dispatcher = new ApplicationEventDispatcher();
        dispatcher.AddListener(@delegate);
        dispatcher.RemoveListener(@delegate);

        // ACT
        dispatcher.Dispatch(new SimpleEvent1());
        dispatcher.Dispose();

        // ASSERT
        Assert.IsFalse(handlerCalled);
    }
\$\endgroup\$
  • \$\begingroup\$ IMO, you are already doing it in the best way (though I'd question your use of @delegate as a variable name!) \$\endgroup\$ – David Arno Feb 12 '16 at 12:15
2
\$\begingroup\$

Test naming

Given/when/then and arrange/act/assert are redundant and both are mostly noise. One would put comments like // ARRANGE etc in a test hundreds of lines long, usually legacy; as a reading help, before refactoring it.

Also method names are misleading. Most obvious one is whereas the method name says WhenAHandlerIsAttached the relevant snippet says:

    // ACT
    dispatcher.Dispatch(new SimpleEvent1());
    dispatcher.Dispose();

Instead the method name should say when_an_event_is_dispatched.

Similarly GivenAnEventToDispatch is also misleading:

    // ARRANGE
    ... @delegate = ...;
    var dispatcher = ...
    dispatcher.AddListener(@delegate);

Instead the method name should say given_an_event_dispatcher_and_an_event_handler_attached_to_it

I like CallsHandler and DoesNotCallHandler better, as you'll see further down; but in the name of consistency, given you have used given/when in the first part of the name, assertion part of the name should say then_the_handler_is_called then_the_handler_is_not_called.

My suggestion is below, I omitted some of the noise, but note the noisy parts are separated and pushed further down:

public class A_dispatcher {

    public void calls_a_handler_after_it_is_attached()
    {
        using(var dispatcher = aDispatcher())
        {
            dispatcher.AddListener(recordCall);

            dispatcher.Dispatch(anEvent());
        }

        Assert.IsTrue(callRecorder.IsCalled());
    }

    public void does_not_call_a_handler_after_it_is_detached()
    {
        using(var dispatcher = aDispatcher())
        {
            dispatcher.AddListener(recordCall);
            dispatcher.RemoveListener(recordCall);

            dispatcher.Dispatch(anEvent());
        }

        Assert.IsFalse(callRecorder.IsCalled());
    }

    private aDispatcher() {
        return new ApplicationEventDispatcher();
    }

    class CallRecorder
    {
        private bool called = false;

        public bool IsCalled() {return called;}

        public void Call()
        {
            this.Called = true;
        }
    }

    ... callRecorder = new CallRecorder();
    ... recordCall = callRecorder.Call;

}

Basic idea is you see this, if you look at he public declarations, e.g. in the class outline panels:

A_dispatcher
    calls_a_handler_after_it_is_attached
    does_not_call_a_handler_after_it_is_detached

Other points if your test case says AHandler your local variable name should not be @delegate.

Is the application event dispatch disposable or not? If you are implementing a Dispose method make the class implement IDisposable, or rename the method, if the class does not follow IDisposable contract.

After you do this you can eliminate the dispathcer.Dispose noise by using using. Or test set-up and clean-up methods.

\$\endgroup\$
  • \$\begingroup\$ Hmm.. An interesting point. I don't use the class outline, but I can see how your convention may read nicely in that. \$\endgroup\$ – Dib Feb 12 '16 at 13:35
  • 1
    \$\begingroup\$ I hadn't thought of using using, I'm more of a try-finally kind of guy, but it does reduce the "noise" as you say. And yes i could use test Setup and Clean-up too. Good call. \$\endgroup\$ – Dib Feb 12 '16 at 13:40
  • \$\begingroup\$ The CallRecorder is a nice touch too! \$\endgroup\$ – Dib Feb 12 '16 at 13:40
  • 1
    \$\begingroup\$ class outline is a demonstration of the effect on readability of reducint noise words given/when/then etc. This naming convention also helps in test runner output. Also look for Kevlin Henney's content on video sites, google, amazon etc. He has useful guidelines, discussions about how one should name tests, what the scope of a unit test should be etc example: Programming with GUTs \$\endgroup\$ – abuzittin gillifirca Feb 12 '16 at 13:41
  • \$\begingroup\$ +1 for the using idea, -1 for replacing the local functions with a class and -1,000,000 for breaking the thread safety of the tests... \$\endgroup\$ – David Arno Feb 12 '16 at 15:38

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.