Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a much needed rewrite of an inherited application ahead of me and have started to sketch out some possible solutions / build some prototypes for different parts of the application.

I'd like to have some feedback if this is a good approach to provide application events.

Goals:

  • Loosely coupled
  • Strictly typed / refactor friendly
  • Declarative
  • Adhere to SRP (Single responsibility principle)
  • Lightweight

All events are classes derived from EventBase. An IoC Container will handle lifetimes of brokers and subscribers. Constructor injection is used. All types implementing IEventSubscriber<> (where T : EventBase) will be automatically registered in the container. My DefaultEventBroker implementation then has an optional IEnumerable<IEventSubscriber<>> dependency, which the container will resolve to all visible subscribers for a given event type. Under the hood a System.Reactive.Subjects.Subject<> (Microsoft Reactive Extensions) will do the heavy lifting.

Essentially I provide two ways to subscribe to events:

Subscribe by setting a dependency on (one or more) IEventBroker<>

public class MyEventListeningComponent
{
    public MyEventListeningComponent(IEventBroker<ProgramStartedEvent> eventBroker)
    {
        eventBroker.Subscribe(e =>
        {
            Console.WriteLine("[MyEventListeningComponent] Program Started: {0}", e.StartDate.ToString());
        });
    }
}

Subscribe by implementing (one or more) IEventSubscriber<>

public class MyEventSubscriber : IEventSubscriber<ProgramStartedEvent>, IEventSubscriber<ProgramEndedEvent>
{
    public void OnNext(ProgramStartedEvent value)
    {
        Console.WriteLine("[MyEventSubscriber] OnNext: Program Started: {0}", value.StartDate.ToString());
    }

    public void OnNext(ProgramEndedEvent value)
    {
        Console.WriteLine("[MyEventSubscriber] OnNext: Program Ended: {0}", value.EndDate.ToString());
    }
}

Any more or less obvious pitfalls? Suggestions?

Side note: IEventSubscriber<> is essentially an IObserver<> without the OnCompleted and OnError methods, because I can subscribe to multiple event types by implementing IEventSubscriber<> with different generic type arguments and I wouldn't be able to get an event type context in OnCompleted and OnError (because those methods do not have generic type arguments). I'm relatively sure I won't be needing those hooks in this scenario anyway.

Edit: Events are raised (or published) via IEventBroker<>

public class MyEventRaisingComponent
{
    public MyEventRaisingComponent(IEventBroker<ProgramStartedEvent> eventBroker)
    {
        eventBroker.Publish(new ProgramStartedEvent() { StartDate = DateTime.Now });
    }
}
share|improve this question
1  
This looks reasonable. But the same pitfall as with every rewrite applies: you're potentially throwing away hours and hours of bug fixes and experience. Proceed with care, unit test along the way and don't rewrite the parts that will be similar to your old code base — you can refactor those safely. –  codesparkle Jan 21 '13 at 21:31
    
The original codebase is insanely complicated for what it does, e.g. custom made script engine and language for mundane tasks, custom file formats for configuration and an unqueryable database structure. Fortunately, for now I only have to port a subset of features. –  Marcel Jan 22 '13 at 6:28
add comment

1 Answer 1

up vote 3 down vote accepted

The code itself looks good, but I would look into providing a single way of subscribing to events, it will make the job easier for other developers reading and changing the code.

The first method of subscribing (via IEventBroker<T> injection) requires subscriber object to be created first, so it may not receive all events that you may expect it to receive. Every time you create an instance of subscriber it adds a new event subscription thus causing potential memory leaks and side effects. You might need to add Unsubscribe method to IEventBroker<T> if you prefer to leave it.

Second method (the one I would prefer) is better suited for application-wide events since it moves the responsibility of subscription to the infrastructure thus avoiding multiple subscriptions and memory leaks (comparing to first option). It is also easier to unit-test since part of responsibilities are taken away from subscriber.

Update based on question update: Publishing messages also looks good. In this case (following my suggestion to leave only second method of event subscription) I would remove Subscribe method from IEventBroker<T> declaration, and refactor it to non-generic IEventPublisher interface with generic method void Publish<T>(T eventToPublish).

share|improve this answer
    
Sorry, I got interrupted after making that edit and before I could answer. Since IEventBroker<> is used to publish events and my default implementation gets all IEventSubscriber<>s passed to it's constructor by the container and hooks those up, there should never be a case were a subscriber misses an event. My DefaultEventBroker also implements IDisposable and takes care of disposing the underlying Subject<>. My container calls Dispose() on any component that implements IDisposable when it's lifetime ends, so there should be no memory leaks (I hope). –  Marcel Jan 22 '13 at 16:41
    
That being said, I like the idea of having an IEventPublisher counterpart to IEventSubscriber<>. I think one could argue that by making IEventPublisher a generic type, a component would state what types of events it generates through it's constructor parameters. On the other hand it could lead to alot of parameters in some cases. –  Marcel Jan 22 '13 at 16:43
    
@Marcel to answer your first comment, "there should never be a case were a subscriber misses an event"... When I mentioned about loosing events I was talking about the Subscribe by setting a dependency on (one or more) IEventBroker<> case: if instance of the component has not been created then subscription code wouldn't have been executed. And that's true that in case of IEventSubscriber<> you won't miss any event. –  almaz Jan 22 '13 at 19:04
    
I'd argue that it's ok that a component only starts listening in on events when it's lifetime begins (assuming subscription happens in the constructor). –  Marcel Jan 23 '13 at 8:48
    
I ended up refactoring IEventBroker like you suggested, but as a generic IEventPublisher<> for the reasons mentioned earlier. If the amount of constructor parameters this leads to get out of hand I might change that to what you suggested as well. –  Marcel Jan 23 '13 at 14:37
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.