News reading application version 2

Question

following the answer I got from t3chb0t last time

News-reading application using the Observer pattern

and also following the example here
https://msdn.microsoft.com/en-us/library/dd990377(v=vs.110).aspx

I implemented a the answer to the following question

Design a kind of kindle fire application where we can subscribe NewsChannel channel and read the NewsChannel from all publishers as a digital format.

Please comment on the implementation of the design pattern observer and the code style. you can ignore the unit test it is just for understanding how to use the code. Thanks

using System;
using System.Collections.Generic;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace DesignPatternsQuestions
{
    /// <summary>
    /// Design a kind of kindle fire application where we can subscribe 
    /// NewsChannel channel and read the NewsChannel from all publishers as a digital format. 
    /// </summary>
    [TestClass]
    public class ObserverPatternTest
    {
        [TestMethod]
        public void ObeserverPatternTest()
        {
            NewsChannel newsChannel1 = new NewsChannel();
            NewsChannel newsChannel2 = new NewsChannel();

            KindleApp kindle = new KindleApp();
            kindle.Subscribe(newsChannel1);
            kindle.Subscribe(newsChannel2);

            newsChannel1.SendMessage(new Image(1920, 1080));
            newsChannel2.SendMessage(new TextMessage("News Channel 2"));

            newsChannel1.EndMessages();
            newsChannel1.SendMessage(new TextMessage("News Channel 1 done"));
        }
    }

    public interface IMessage
    {
        string Print();
    }

    public class TextMessage : IMessage
    {
        private readonly string _text;
        public TextMessage(string text)
        {
            _text = text;
        }
        public string Print()
        {
            return _text;
        }
    }

    public class Image : IMessage
    {
        private readonly uint _width;
        private readonly uint _height;
        public Image(uint width, uint height )
        {
            _width = width;
            _height = height;
        }
        public string Print()
        {
            return string.Format("Image width:{0} height {1}", _width, _height);
        }
    }

    /// <summary>
    /// this class handles all of the different observers, observers listen to IObservables..
    /// </summary>
    /// <typeparam name="T"></typeparam>
    public class NewsChannel : IObservable<IMessage>
    {
        private readonly List<IObserver<IMessage>> _observers;
        public NewsChannel()
        {
            _observers = new List<IObserver<IMessage>>();
        }
        public IDisposable Subscribe(IObserver<IMessage> observer)
        {
            if (!_observers.Contains(observer))
            {
                _observers.Add(observer);
            }
            return new Unsubscriber<IMessage>(_observers, observer);
        }
        /// <summary>
        /// send a message of certain type to all of the observers
        /// </summary>
        /// <param name="message"></param>
        public void SendMessage(IMessage message)
        {
            foreach (var observer in _observers)
            {
                if (message != null)
                {
                    observer.OnNext(message);
                }
                else
                {
                    observer.OnError(new ArgumentNullException());
                }
            }
        }

        public void EndMessages()
        {
            foreach (var observer in _observers)
            {
                observer.OnCompleted();
            }
            _observers.Clear();
        }
    }
    /// <summary>
    /// this also can be a private class inside the NewsChannel class
    /// </summary>
    /// <typeparam name="T"></typeparam>

    public class Unsubscriber<T> : IDisposable
    {
        private List<IObserver<T>> _observers;
        private IObserver<T> _observer;
        public Unsubscriber(List<IObserver<T>> observers, IObserver<T> observer)
        {
            this._observers = observers;
            this._observer = observer;
        }
        public void Dispose()
        {
            if (_observer != null && _observers.Contains(_observer))
            {
                _observers.Remove(_observer);
            }
        }
    }

    public class KindleApp : IObserver<IMessage>
    {
        private IDisposable _unsubscriber;

        public virtual void Subscribe(IObservable<IMessage> provider)
        {
            if (provider != null)
            {
                _unsubscriber = provider.Subscribe(this);
            }
        }
        public virtual void Unsubscribe()
        {
            _unsubscriber.Dispose();
        }
        //print the message
        public void OnNext(IMessage value)
        {
            Console.WriteLine(value.Print());
        }
        public void OnError(Exception error)
        {
            Console.WriteLine("can't handle error");
        }
        public void OnCompleted()
        {
            Console.WriteLine("on complete");
            this.Unsubscribe();
        }
    }
}

Answer 1

You were interested in a review in both the pattern as code style.

_{Note: I'm addressing subscribers also as listeners and observers in this review.}

---

Code Style

• Prefer var when the type of the instance is known: NewsChannel newsChannel1 = new NewsChannel(); -> var newsChannel1 = new NewsChannel();
• Use expression-bodied members for compact code blocks. For instance public string Print() => _text;
• Use string-interpolation for readability: $"Image width:{_width} height {_height}"
• Use a blank line between methods.
• Don't abuse /// <summary> for remarks. this also can be a private class inside the NewsChannel class could be inserted in a /// <remarks> block instead.

---

Observer Pattern

• You specifically want to implement the observer pattern yourself, so I can understand you are not using the built-in observer pattern event. However, in production code, you'd always want to take advantage of native language support.

• You provide a custom unsubscriber to each listener that subscribes. This seems a bit heavy for my taste. Why not just provide an Unsubscribe method? You now have to cope with additional complexity:

  • (1) you return a new unsubscriber every time, even if already subscribed
  • (2) you don't allow a listener to subscribe more than once, which may be as intended, but also limits possibilities
  • (3) a subscription attempt does not get a response whether subscription was registered
  • (4) you have free floating unsubscribers that could keep memory alive, even when you dispose the channel
• SendMessage loops over the observers when notifying them. Note that an observer can unsubscribe on notification, causing the enumeration to loop over a modified collection. This throws a runtime error. Take a snapshot of the current observers before notifying them.
• EndMessages clears the observers after notifying them, but should also remove any unsubscriber it created (if you still decide to use this pattern).
• I expect NewsChannel to be IDisposable, notify all observers and clear all resources. KindleApp should also implement IDisposable and unsubscribe from all subscribed channels.
• In your example, KindleApp subscribes to multiple channels, but only stores a single unsubscriber. Method Unsubscribe only unsubscribes from the last channel. Surely, this cannot be as intended. Either keep track of all unsubscribers, or of the channels directly (in my suggested refactoring you should call Unsubscribe directly on the channels).