SerialPort C# Class

Question

Originally I submitted a question here: SerialPort class for a library

I cleaned up my code a bit and rewrote a few things. I've had a number of problems along the way and I've still not gotten to test this class, but I'd like to ask if there's anything major I can do to improve it.

using System;
using System.IO.Ports;
using System.Text;
using System.Threading;
namespace SerialPortSharp
{
 public sealed class SerialPortConn : IDisposable
 {
 private readonly SerialPort serialPort;
 private readonly string returnToken;
 private readonly string hookOpen;
 private readonly string hookClose;
 private bool disposed;
 public SerialPortConn(
 string comPort = "Com1",
 int baud = 9600,
 Parity parity = Parity.None,
 int dataBits = 8,
 StopBits stopBits = StopBits.One,
 string returnToken = "> ",
 string hookOpen = "",
 string hookClose = ""
 )
 {
 this.serialPort = new SerialPort(comPort, baud, parity, dataBits, stopBits)
 {
 ReadTimeout = 1000,
 RtsEnable = true,
 DtrEnable = true
 };
 this.returnToken = returnToken;
 if (hookOpen == "")
 this.hookOpen = null;
 else
 this.hookOpen = hookOpen;
 if (hookClose == "")
 this.hookClose = null;
 else
 this.hookClose = hookClose;
 }
 public bool OpenConnection()
 {
 if (this.disposed)
 {
 throw new ObjectDisposedException(this.GetType().Name, "Cannot use a disposed object.");
 }
 try
 {
 this.serialPort.Open();
 this.serialPort.DiscardInBuffer();
 bool hooked = false;
 if (hookOpen != null)
 {
 hooked = this.Hook();
 }
 else
 {
 hooked = true;
 }
 if (hooked)
 {
 return true;
 }
 else
 {
 return false;
 }
 }
 catch
 {
 return false;
 }
 }
 public bool CloseConnection()
 {
 if (this.disposed)
 {
 throw new ObjectDisposedException(this.GetType().Name, "Cannot use a disposed object.");
 }
 try
 {
 bool unhooked = false;
 if (hookClose != null)
 {
 unhooked = this.UnHook();
 }
 else
 {
 unhooked = true;
 }
 if (unhooked)
 {
 Thread.Sleep(100);
 this.serialPort.ReadLine();
 this.serialPort.DiscardInBuffer();
 this.serialPort.Close();
 this.serialPort.Dispose();
 return true;
 }
 else
 {
 return false;
 }
 }
 catch
 {
 return false;
 }
 }
 public void Dispose()
 {
 if (this.disposed)
 {
 throw new ObjectDisposedException(this.GetType().Name, "Cannot dispose of a disposed object.");
 }
 var closed = this.CloseConnection();
 if (closed)
 {
 this.disposed = true;
 }
 else
 {
 throw new Exception("Error! Could not close port!");
 }
 }
 private bool Hook()
 {
 if (this.disposed)
 {
 throw new ObjectDisposedException(this.GetType().Name, "Cannot use a disposed object.");
 }
 try
 {
 this.serialPort.Write(hookOpen + "\r");
 Thread.Sleep(100);
 this.serialPort.DiscardInBuffer();
 return true;
 }
 catch
 {
 return false;
 }
 }
 private bool UnHook()
 {
 if (this.disposed)
 {
 throw new ObjectDisposedException(this.GetType().Name, "Cannot use a disposed object.");
 }
 try
 {
 this.serialPort.Write(hookClose + "\r");
 Thread.Sleep(100);
 this.serialPort.DiscardInBuffer();
 return true;
 }
 catch
 {
 return false;
 }
 }
 private string HookTest(string serialCommand)
 {
 if (this.disposed)
 {
 throw new ObjectDisposedException(this.GetType().Name, "Cannot use a disposed object.");
 }
 try
 {
 this.serialPort.Write(serialCommand + "\r");
 Thread.Sleep(100);
 bool loop = true;
 string output = "";
 while (loop)
 {
 output += this.serialPort.ReadExisting();
 if (output.EndsWith(this.returnToken))
 {
 break;
 }
 }
 return output;
 }
 catch (TimeoutException e)
 {
 throw new Exception("Connection failed. Read timed out.");
 }
 }
 public string WriteConnection(string serialCommand, bool isSafe = false)
 {
 if (this.disposed)
 {
 throw new ObjectDisposedException(this.GetType().Name, "Cannot use a disposed object.");
 }
 if (isSafe)
 {
 var output = this.HookTest(serialCommand);
 return output;
 }
 else
 {
 try
 {
 this.serialPort.Write(serialCommand + "\r");
 Thread.Sleep(100);
 return this.serialPort.ReadExisting();
 }
 catch (Exception e)
 {
 throw new Exception("Connection failed. Timed out.");
 }
 }
 }
 }
}

(削除) As a note, a couple returned strings contain "#!". I want to return the entire exception instead of just returning false (signifying the write failed). I just added something that easily differentiates it from the default strings returned by reading whatever's returned by the SerialPort() (削除ここまで)

Edit: I tried to implement svick's recommendation and changed Open/CloseConnection to check if hook/unhook is successful.

EDIT 2: I've changed the code about lately. I finally freed up a device for testing purposes and it works. I tested 400+ queries and only gotten several errors related to a device quirk I had to work out in the commands I was submitting. I'd love to hear any further suggestions.

• svick: "I want to return the entire exception" You really shouldn't do that. If you can't handle an exception, let it bubble up. In general catch (Exception ex) or just catch (with no exception specified) is a big code smell. You should always catch only the specific exceptions that you know how to handle.
• DanteTheEgregore: @svick I've made an edit. I'd like to clarify that the most common exception is a TimeoutException which occurs when the read fails out. These really shouldn't be fatal. I'm just returning the strings with an identifier so I can throw them in a log at this point.
• DanteTheEgregore: @svick So it's not that they're unhandled, I'm just expecting Timeouts so I catch them, spit them out as a string, and throw them in a log. Is that a bad idea?
• svick: I think so. It should be up to the method that calls WriteConnection() to decide what to do with normal output and what to do with an error, it shouldn't be up to HookTest(). A reasonable solution would be to catch the TimeoutException and then throw a new exception of some specific type, so that the caller doesn't need to know about implementation details.
• DanteTheEgregore: @svick I tried editing the code to reflect what you recommended.

Answer 1

Not sure if still relevant, but here you go:

1. Consider refactoring if (this.disposed) { } into a method like ThrowIfDisposed(). If you ever want to change the dispose method or add logging or whatnot then you have to change only one place. Code duplication should be avoided even for trivial things like that.

2. You swallow exceptions in Hook and Unhook and return a bool which tells you nothing except that it went wrong. Same in OpenConnection and CloseConnetion. A lot of information is thrown away which will be useful for troubleshooting should the need arise. In general I would consider that a bad idea.

3. Replace the magic constant Thread.Sleep(100); either with a const definition or even a setting you can tweak. If you make it a setting please make it a TimeStamp - I personally find all that code which scatters around ints with implicit units very annoying. I often have to look up what it means because it's not always that obvious - seconds, milliseconds, ..?

Answer 2

Usability / Testability

The problem of this class is it has a dependency on SerialPort. This is very hard to test. You'd either need a serial port or a USB-2-Serial-Converter. Also note that these converters react differently on connection issues than a serial port.

The solution is to create an interface IStreamResource that provides all the methods you need.

public interface IStreamResource : IDisposable
{
 void Open();
 void DiscardInBuffer();
 string ReadLine();
 void Write(string buffer);
 // ..
}

You can create an adapter/bridge for SerialPort.

public class SerialPortAdapter : IStreamResource
{
 public SerialPort Source { get; }
 public SerialPortAdapter(SerialPort source) => SerialPort = source; // check not null..
 // IStreamResource impl ..
}

Havig done this, you could also make adapters for TcpClient, UdpClient, UnitTestAdapter, or any other stream resource.

You could then change your class to use the interface.

public SerialPortConn(IStreamResource streamResource)
{
 StreamResource = streamResource; // check not null ..
}

You could allow for overloads for common stream resources.

public SerialPortConn(SerialPort serialPort)
 : this (new SerialPortAdapter(serialPort)
{
}