Hardware resource Open/Close methods

So I'm making a program that is a serial port wedge. In other words I want it to listen to a serial port. If data comes in, wait a given amount of time, then play a sound and finally send the data out.

All of those things have a abstract class associated with it. I'm thinking that I might eventually want to monitor USB:HID things so I made a IInput class which has abstract methods of Open, Close, ToXmlString, ParseXmlString and has a event of DataAvailable. I extend that class with ComPortInput.

This is the question of it: Do I subscribe to my data event in the Open method, then unsubscribe in close? Or should I just have it in the initializer. I have a GUI that a user can change any of the 4 elements on this wedge at any time. But I'm not sure what would be the cleanest way of doing this. I'm thinking that the GUI would always create a new instance of my ComPortInput class. Part of me wants to force that GUI to have another method like Check Input. which in this case if the COM port was invalid or busy it would return false. Then instead of making a ComPortInput I would then want a NoInputInput class (for when I don't want to port forward) If I did it this way then I could get rid of my null checks on the serial port (Much desired) but is that too tricky? (as in too hard to read?) I'm thinking that if i do that I could put in my constructor of ComPortInput to Assert not null of the SerialPort. To me this looks better

public ComPortInput(SerialPort sp, int delay) : base(delay)
{
    if (sp == null)
        throw new System.ArgumentNullException("Serial port can not be null");

    LibraryTrace.Start("ComPortInput", delay);

    serialPort = sp;
    serialPort.DataReceived += sp_DataReceived;

    LibraryTrace.End("ComportInput");
}
~ComPortInput()
{
    serialPort.Dispose();
}

My thoughts on this way, though, is that it might be hard to have a abstract CheckValidity... Hmm just thought as I was typing what if I put a `params objec

As Jesse C. Slicer mentioned, because your class seems to encapsulate an IDisposable object (assuming serialPort would be a private field - this would be clearer if you named it _serialPort), it's asking to implement the IDisposable interface.

Thus, instead of a destructor, you would simply have a Dispose method:

public void Dispose()
{
    serialPort.Dispose();
}

Another thing is this LibraryTrace whose Start and End static methods are a testability hinderance. If it's at all possible, I would wrap that with an interface like ILibraryTrace and implement it in a LibraryTraceWrapper : ILibraryTrace class that exposes the static LibraryTrace methods as instance methods - and inject it into the constructor as a dependency - same goes for the SerialPort:

private readonly ILibraryTrace _trace;
private readonly ISerialPort _serialPort;

public ComPortInput(ILibraryTrace tracer, ISerialPort sp, int delay) : base(delay)
{
    // ...
}

This way you could unit test your class and inject mocks as needed.

As for your non-null assertion, that's called a guard clause and it's good because you're failing fast; by throwing in the constructor you thwart the instantiation of your class without a serial port.

I don't know if it's a typo or if it could be an actual issue, but LibraryTrace.Start and LibraryTrace.End are not working off the same string parameter value (you have "ComPortInput" and "ComportInput" - those are not the same string in C#). Given you're passing in the type's name, I would seriously consider passing in typeof(this).Name instead of a magic string. This shields both calls from an eventual rename refactoring.

StackExchange Code Review Q#24051, answer score: 2