3
\$\begingroup\$

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 object[] args for CheckValidity and just have the comport i could try a few things and return true or false. Then if I ever decide to accept other types of input I could do something similar (such as if it was USB:HID I could accept the PID/VID) What do you guys think? Am I on the right track?

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Yeah I hated updating those strings so I moved to reflection for those. End result is much better as I only have Start() ad End() now and Reflection puts in the name of the function for me. I know its a performance hit, but I use this method on another library that requires speed and it's not been hindering it enough to delete all logging. Thank your input. \$\endgroup\$ – Robert Snyder Nov 21 '13 at 5:23
  • 1
    \$\begingroup\$ Have you tried typeof()? I'm guessing you're using GetType() - if the type is in the same assembly, typeof() performs better (per MSDN - just closed the tab...sorry) \$\endgroup\$ – Mathieu Guindon Nov 21 '13 at 5:24
  • 1
    \$\begingroup\$ @RobertSnyder feel free to mark your [rather old] question as answered, as you [probably] know we're on a mission :) \$\endgroup\$ – Mathieu Guindon Nov 21 '13 at 5:26
  • \$\begingroup\$ HA.. i didn't know that. I had to look up my code and apparently I had C++ on the mind. public static string __FUNCTION__ {get{return new StackFrame(3).GetMethod().Name; } hahahah. I did not know about typeof(this).Name I'll have to give that a shot if performance becomes an issue } \$\endgroup\$ – Robert Snyder Nov 21 '13 at 5:31
  • \$\begingroup\$ You're unwinding the stack? That's another quite significant performance hit! \$\endgroup\$ – Mathieu Guindon Nov 21 '13 at 5:41

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.