patterncsharpMinor
SerialPort C# Class
Viewed 0 times
classserialportstackoverflow
Problem
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)
{
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)
{
Solution
Not sure if still relevant, but here you go:
-
Consider refactoring
-
You swallow exceptions in
-
Replace the magic constant
-
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.-
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.-
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, ..?Context
StackExchange Code Review Q#31483, answer score: 3
Revisions (0)
No revisions yet.