3
\$\begingroup\$

In order to implement the Observer pattern in C#, one of the ways to go (at least, the one I chose) is to make classes that implement the IObservable<T> for the observable objects and the IObserver<T> for the observers.

In a project of mine I created a base class from which every observable inherits:

public class Observable<T> : IObservable<T>
{
    private SubscriptionManager<T> _subscriptionManager;

    public Observable()
    {
        _subscriptionManager = new SubscriptionManager<T>(
            new List<IObserver<T>>());
    }

    public IDisposable Subscribe(IObserver<T> observer)
    {
        _subscriptionManager.Subscribe(observer);

        return _subscriptionManager;
    }

    public void Notify(T obj)
    {
        _subscriptionManager.Notify(obj);
    }
}

and an IDisposable class that manages the subscriptions to the observable object:

public class SubscriptionManager<T> : IDisposable
{
    private ICollection<IObserver<T>> _observers;
    private IObserver<T> _observer;

    public SubscriptionManager(ICollection<IObserver<T>> observers)
    {
        if (observers == null)
        {
            throw new ArgumentNullException("observers");
        }

        _observers = observers;
    }

    public void Subscribe(IObserver<T> observer)
    {
        _observers.Add(observer);
        _observer = observer;
    }

    public void Notify(T obj)
    {
        foreach (var observer in _observers)
        {
            observer.OnNext(obj);
        }
    }

    public void Dispose()
    {
        _observers.Remove(_observer);
    }
}

Do you see anything that can be done more efficiently, or in a better way? Do you see anything else that may be wrong?

\$\endgroup\$
4
\$\begingroup\$

Here's a problem:

var o = new Observable<Unit>();
var a = o.Subscribe(_ => Console.WriteLine("A"));
var b = o.Subscribe(_ => Console.WriteLine("B"));
a.Dispose();
o.Notify(Unit.Default);

This prints

A

Disposing of a removed the wrong observer.


I'd recommend reading Why shouldn't I implement IObservable<T>?

The reason you shouldn't implement IObservable<T> is the same reason you don't usually implement IEnumerable<T>, is that somebody has most likely already built the thing you want.

\$\endgroup\$
  • \$\begingroup\$ Thanks to your answer I noticed a big flaw in the SubscriptionManager<T>. Basically, the Dispose() removes only the last observer only the first time that it's called. Thanks a lot. +1 \$\endgroup\$ – Gentian Kasa Oct 16 '15 at 6:49
3
\$\begingroup\$

You have a problem with your disposing. The Dispose method is like cancer. Once it's somewhere, it has to be in your whole object graph. Because if I dispose my Observable<T>, wouldn't I want the SubscriptionManager<T> to be disposed too?

So, Observable<T> should implement IDisposable too and call _subscriptionManager.Dispose().

Since your SubscriptionManager<T> is shared between all your Observer<T> (from what I understood), shouldn't its Dispose clear the list of Observer<T>?

Also, maybe you'd need a method Unregister(IObserver<T>), so that if I don't want to be notified by you anymore I can unregister myself from your observers.

Overall, I must admit I feel there's something wrong with your approach. It'd be good to see what the Observer<T> class looks like (because I feel you should have one. Some code in IObserver<T> will be repeated in all your child classes otherwise). But you have a good at disposing of the observers, your naming convention is perfect and you're close from a very good Observable/Observer pattern! :)

\$\endgroup\$
  • \$\begingroup\$ Ignore my previous comment, I will need to go update my answer \$\endgroup\$ – moarboilerplate Oct 15 '15 at 20:32
  • \$\begingroup\$ @TopinFrassi Great tips, thanks. I'll definitely create the Unregister(IObserver<T>) method (it's actually one that might come in handy) and work on the Dispose part. Regarding the Observer<T> class, I haven't written it because I've had no need for one yet. Regarding your feeling of something being wrong, I guess I pinned down what it is (I noticed it also thanks to @mjolka's answer). +1 \$\endgroup\$ – Gentian Kasa Oct 16 '15 at 6:55
3
\$\begingroup\$

As you flesh out your implementation further and have more design decisions to make, the efficacy of your object model will prove itself, so don't worry too much about it up front.

A few things:

  • I wouldn't make an Observable<T> class. That's kind of asking for trouble. Just make concrete implementations of your observables to keep it simple. If you're worried about duplicating code, don't worry about it until you have a code duplication problem. Once you do, create an abstract base class that is more specific to the duplicated code so it is precisely defined by that functionality and not as a generic "Observable Thing" that could potentially become a dumping ground, or a pain point for future observable implementations that need to be shoehorned into being an Observable Thing.
  • You can make your properties readonly so you can enforce that they only get set in the constructor
  • Use an IEnumerable for the manager's constructor parameter because only the manager is going to need to treat this set of objects as a collection. I'd just keep it simple and use a private List under the hood, because you're not exposing the collection and you can initialize a list from an IEnumerable. If you're concerned about it being lazily evaluated, don't worry, the List initialization will enumerate the collection.
  • If you look at the implementation provided on MSDN you can see a deviation from the implementation where your subscription manager is responsible for calling OnNext(). Notification should be the job of the observer directly referencing its observables instead of proxying through another object.
  • In the referenced example, also notice that instead of the manager being a property on the class, individual instances of the unsubscriber are tracked in order to unsubscribe individual observers on demand, not just clear the list.
\$\endgroup\$
  • \$\begingroup\$ great feedback. I'll definitely make good use of it. +1 \$\endgroup\$ – Gentian Kasa Oct 16 '15 at 7:01

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.