Class simulating a connection among two pins

Question

Following this post and the one before that, here I am asking for a review on another piece of code: the connection among two pins. As usual, be merciless in your review. Here it goes:

public class PinConnection : IObserver<Pin>
{
 #region variables
 private Pin _from;
 private Pin _to;
 private IDisposable _fromUnsubscriber;
 private IDisposable _toUnsubscriber;
 #endregion
 #region properties
 /// <summary>
 /// Get or Set the label of this PinConnection object.
 /// </summary>
 public string Label { get; set; }
 public Pin From
 {
 get
 {
 return _from;
 }
 set
 {
 _from = value;
 if (_fromUnsubscriber != null)
 {
 _fromUnsubscriber.Dispose();
 }
 if (_from != null)
 {
 _fromUnsubscriber = _from.Subscribe(this);
 _from.Notify(_from);
 }
 }
 }
 public Pin To
 {
 get
 {
 return _to;
 }
 set
 {
 _to = value;
 if (_toUnsubscriber != null)
 {
 _toUnsubscriber.Dispose();
 }
 if (_to != null)
 {
 _toUnsubscriber = _to.Subscribe(this);
 _to.Notify(_to);
 }
 }
 }
 #endregion
 #region constructors
 public PinConnection()
 {
 Code = string.Empty;
 Label = string.Empty;
 }
 public PinConnection(Pin from)
 : this()
 {
 From = from;
 }
 public PinConnection(Pin from, Pin to)
 : this(from)
 {
 To = to;
 }
 #endregion
 public void OnCompleted()
 {
 throw new NotImplementedException();
 }
 public void OnError(Exception error)
 {
 throw error;
 }
 public void OnNext(Pin value)
 {
 if (value.Equals(From) && To != null)
 {
 To.Value = value.Value;
 }
 }
}

Answer 1

I'm not sure how I feel about this.

public void OnCompleted()
{
 throw new NotImplementedException();
}

In my opinion, if you're going to implement a contract, you should fulfill the contract, even if that means doing nothing at all. There's little more frustrating than getting an object that you believe implements an interface only to find out that it doesn't really implement it. Oh no, this implementation throws an exception when I call that method. Now, my nice clean code that, previously, didn't care what concrete type it was passed has to type check it's args to make sure it's not an instance of this class you've created.

Ok. So I do know how I feel about it. I hate it. I really really hate it. I'd much rather that you fulfill the contract by truly not implementing it.

/// <summary>
/// Does nothing. Not implemented for reasons.
/// </summary>
public void OnCompleted() { }

Now my code doesn't have to type check for your class anymore. Even better, I know why you've not implemented this part of the contract and decided that this method should just do nothing at all when called.

Of course, if it's not going to do anything, then we can hide it from the public interface of the concrete type by explicitly implementing the interface.

/// <summary>
/// Does nothing. Not implemented for reasons.
/// </summary>
public void IObserver.OnCompleted() { }

• Thomas Eyde: Good points on the contract, and hiding the unimplemented methods.
• Gentian Kasa: Good points. I was actually thinking about the first point but wasn't sure if it was considered a good practice
• RubberDuck: @GentianKasa don't take my answer as anything canonical. It's just my personal opinion on the matter. I honestly wasn't sure about it when I started writing my answer, but the more I thought about it, the more I thought "I wouldn't want to consume this class the way it is". Side note: mind sharing the link to your project?
• Gentian Kasa: @RubberDuck didn't take it for anything canonical, it was just better than what I was doing, and the fact that I already thought about it and other people agree (while no disagreement came forth) is proof enough :) As for the project, you can find it here, but it's still in its early stage, so it's pretty blank
• RubberDuck: Thanks @GentianKasa I'll check it out. I've been toying around with some embedded dev, so you peeked my interest.