A library that reads incoming scale data from USB

Question

I made a simple C# library which detects incoming data from 1 or 2 scales plugged into a computer's usb.

My main concerns:

1. I feel like I should potentially be using events/delegates to process the data, but I'm not sure how to approach this so would appreciate any advice
2. My error-handling can probably be improved
3. The application is not s flexible as it could be (e.g. I can't customize the serial port initialisation, or acocunt for different formats of data or number of scales attached to the machine)
4. I am returning the read weights as a Microsoft.VisualBasic.Collection, as the library is currently being called by a macro in MS Access. However, if this was to be used by any other application, perhaps it would be better to return a System.Collections.Generic.IEnumerable

ScaleReader.cs

The main class to be instantiated and called. It initialises ports in the machine and then we can call the ReadAllWeights method to get the data from any scales that are plugged in

public class ScaleReader
{
    private List<Port> ports;
    private bool multiScale;

    /// <summary>
    /// The constructor simply finds all COM ports which are in use and initialises 
    /// each of them with InitialisePort(string portName).
    /// </summary>
    public ScaleReader()
    {
        ports = PortHelper.GetPorts();
        multiScale = (ports.Count > 1);
    }

    /// <summary>
    /// Iterates through all COM ports stored in the Ports List field, and
    /// calls ReadWeight(SerialPort port) for each one. The values output
    /// from ReadWeight are returned as a Microsoft.VisualBasic.Collection. 
    /// </summary>
    /// <remarks>
    ///     The reason why this method returns a VisualBasic collection rather
    ///     than a list or anything else is so that the items can be retrieved
    ///     from inside an MS-Access macro. 
    /// </remarks>
    /// <returns></returns>
    public Collection ReadAllWeights()
    {
        var weights = new Collection();

        ports.ForEach(port 
            => weights.Add(port.ReadWeight(multiScale)));

        // If there is a mismatch between our weights and the ports, add some 
        // 0 weights as something has gone wrong (add 2 in case of multiweight) 
        if (weights.Count != ports.Count)
        {
            weights.Add(0);
            weights.Add(0);
        }

        return weights;
    }
}

PortHelper.cs

Contains some static methods to assist in dealing with the computer's ports - GetPortNames (self-explanatory) and GetPorts, which uses the port names to return a list of Port objects

class PortHelper
{
    /// <summary>
    /// Returns a list of port names retrieved from SerialPort.GetPortNames()
    /// </summary>
    /// <returns></returns>
    private static List<string> GetPortNames()
    {
        var portNameList = new List<string>();
        try
        {
            var portNames = SerialPort.GetPortNames();
            portNameList = portNames.ToList();
        }
        catch (System.ComponentModel.Win32Exception w32Ex)
        {
            var error = new Error();
            error.Log(w32Ex, "Could not enumerate ports", 
                "Failed to call SerialPort.GetPortNames() in PortHelper.cs");
        }
        catch (ArgumentNullException nEx)
        {
            var error = new Error();
            error.Log(nEx, "Could not convert ports to list", 
                "Failed to call ToList() on SerialPort.GetPortNames() in PortHelper.cs");
        }
        return portNameList;
    }

    /// <summary>
    /// Gets a list of ports from GetPortNames() and then returns
    /// a list of ports based on those names
    /// </summary>
    /// <param name="portNames"></param>
    /// <returns></returns>
    internal static List<Port> GetPorts()
    {
        var portNames = GetPortNames();
        var ports = new List<Port>();

        if (portNames.Count < 1)
        {
            MessageBox.Show("Could not find any connected scales.");
        }
        else
        {
            portNames.ForEach(portName =>
                ports.Add(new Port(portName)));
        }
        return ports;
    }
}

port.cs

This represents an instance of a SerialPort, and contains some methods used by the scale reader, e.g. ReadWeight - which attempts to process the incoming string data. There is some concern that this depends on the configuration of the scale/port, but I'm not sure what the best way would be to handle this. The Multiscale option specified whether or not the computer is expected to have more than 1 scale plugged in

class Port
{
    public SerialPort COMPort;
    public readonly string Name;
    const decimal ErrorValue = -999;

    public Port(string portName)
    {
        Name = portName;
        COMPort = new SerialPort()
        {
            PortName = Name,
            BaudRate = 9600,
            Parity = Parity.None,
            DataBits = 8,
            StopBits = StopBits.One,
            Encoding = Encoding.ASCII,
            Handshake = Handshake.None,
            RtsEnable = true,
            ReceivedBytesThreshold = 1,
            ReadTimeout = 3000
        };
    }

    /// <summary>
    /// Open the given port and get the weight output, returning it as 
    /// a decimal.
    /// </summary>
    /// <param name="port"></param>
    /// <returns></returns>
    public decimal ReadWeight(bool multiScale)
    {
        var scaleDecimal = ErrorValue;
        var giveup = 0;
        Open();

        if (COMPort.IsOpen)
        {
            using (COMPort)
            {
                while (scaleDecimal == ErrorValue && giveup < 20)
                {
                    var scaleStringData = ReadData(multiScale);
                    scaleDecimal = ConvertStringToDecimal(scaleStringData);
                    giveup++;
                }
            }
        }
        else
        {
            var error = new Error();
            error.Log(new Exception("the port wa s not open when checking it to read"),
                "Port is not open", $"{COMPort.PortName}");
        }

        if (COMPort.IsOpen)
        {
            Close();
        }

        return scaleDecimal;
    }

    private decimal ConvertStringToDecimal(string scaleStringData)
    {
        if (!Decimal.TryParse(scaleStringData, out decimal outDecimal))
            outDecimal = 0;
        return outDecimal;
    }

    /// <summary>
    /// Opens the current Port's SerialPort (COMPort)
    /// </summary>
    private void Open()
    {
        try
        {
            COMPort.Open();
        }
        catch (InvalidOperationException) // The port is already open
        {
            return;
        }
        catch (ArgumentException argEx)
        {
            var error = new Error();
            error.Log(argEx, 
                $"Could not open port {COMPort.PortName}", 
                $"Port name: {Name} COMPort name: {COMPort.PortName}");
        }
        catch (Exception ex)
        {
            var error = new Error();
            error.Log(ex, $"Could not open port {COMPort.PortName}", 
                $"IsOpen: {COMPort.IsOpen}");
        }
    }
    private void Close()
    {
        try
        {
            COMPort.Close();
        }
        catch (Exception ex)
        {
            var error = new Error();
            error.Log(ex, $"Could not close port {COMPort.PortName}",
                $"IsOpen?: {COMPort.IsOpen}");
        }
    }

    /// <summary>
    /// Reads the output data of a given port and processes it to retrieve the
    /// latest reading, returning it as a string.
    /// </summary>
    /// <param name="port"></param>
    /// <returns></returns>
    private string ReadData(bool multiScale)
    {
        var scaleData = string.Empty;

        if (multiScale)
        {
            scaleData = ReadMultiPortData();
        }
        else
        {
            scaleData = ReadSinglePortData();
        }

        return CleanString(scaleData);
    }
    private string ReadSinglePortData()
    {
        var scaleData = COMPort.ReadLine();
        var giveup = 0;
        while (string.IsNullOrEmpty(scaleData) & giveup < 1000)
        {
            scaleData = COMPort.ReadLine();
            giveup++;
        }
        return scaleData;
    }
    /// <summary>
    /// Data from the multi-scale machines looks different to that
    /// sent from a single-scale machine
    /// </summary>
    /// <returns></returns>
    private string ReadMultiPortData()
    {
        //var scaleData = COMPort.ReadExisting();
        var scaleData = string.Empty;
        var giveup = 0;
        while (string.IsNullOrEmpty(scaleData) && giveup < 10000)
        {
            if (COMPort.BytesToRead > 0)
            {
                scaleData = COMPort.ReadExisting();
            }
            giveup++;
        }

        return ProcessMultiPortData(scaleData);
    }
    private string ProcessMultiPortData(string scaleData)
    {
        try
        {
            // TEST

            var scaleDataArray = scaleData.Split('\r');
            scaleData = scaleDataArray[scaleDataArray.Length - 2];

            //// Find the penultimate \r
            //var ultimateIndex = scaleData.LastIndexOf("\r");
            //var penUtimateIndex =
            //    ultimateIndex > 0
            //        ? scaleData.LastIndexOf("\r", ultimateIndex - 1)
            //        : -1;

            //// Remove everything before the penultimate /r
            //scaleData = scaleData.Substring(penUtimateIndex);
            //// Remove everything after the ultimate /r
            //scaleData = scaleData.Substring(0, scaleData.LastIndexOf("\r"));
        }
        catch (Exception)
        {
            // This happens so often that it's probably not worth logging it ...
            //var error = new Error();
            //error.Log(ex, "Failed to process multiscale output data. Now returning scaleData as is", $"scaleData: {scaleData}");
            return scaleData;
        }

        return scaleData;
    }

    /// <summary>
    /// Calls a regex on the given string to return the string with everything
    /// but +, - and [0-9]s removed.
    /// </summary>
    /// <param name="scaleData"></param>
    /// <returns></returns>
    private string CleanString(string scaleData)
    {
        var pattern = "[^0-9.+-]";
        if (!string.IsNullOrEmpty(scaleData))
        {
            try
            {
                scaleData = Regex.Replace(scaleData, pattern, "");
            }
            catch (ArgumentException aEx)
            {
                var error = new Error();
                error.Log(aEx, $"Error when calling Regex.Replace on stringstring {scaleData}", 
                    $"Regex: {pattern}, scaleData: {scaleData}");
            }
            return scaleData;
        }
        else
        {
            return "0";
        }
    }
}

Answer 1

ScaleReader.cs

If you do not prefix private fields with _ then IMHO it's better to get used to always use the this.fieldName style, it helps to quickly understand when you're accessing a local variable (and it prevents small mistakes when you need a parameter with the same name).

ports and multiscale variables should be readonly, they're written only in ctor.

In (ports.Count > 1) you have unneeded parenthesis and, I think, it's more clearly expressed by ports.Any().

Given its usage I'd declare ports as IEnumerable<Port> (or another read-only collection) to prevent any unwanted change.

In ReadAllWeights(), I don't know the exact type of weights but if that type has an AddRange() method you may rewrite it as:

weights.AddRange(ports.Select(port => port.ReadWeight(multiScale)));

Or probably even directly:

return new Collection(ports.Select(port => port.ReadWeight(multiScale)));

However few lines later you do something extremely dangerous. I don't know how the two collections may possibly have a different number of items because there isn't any filter, if something that bad happen (possibly because of some strange thing inside Collection.Add() implementation) then you should clearly document WHY and WHEN it may happen and you should explicitly check for the exact condition:

// If foo and bar happen then baaz may cause Collection.Add()
// to do insert null duplicates (but at most, because of XYZ, there
// might be only two less).
if (weights.Count == ports.Count - 2) 
{
    // ...
}

PortHelper.cs

You're correctly catching different exceptions however you probably should NEVER catch ArgumentException (and derived classes). It's a programming error, not a run-time exceptional condition (but as usual there are...exceptions to this rule). If one variable might be null then make it explicit and add a check for that. More than that: I do not see a single reason in that code for this exception to be thrown. If SerialPort.GetPortNames() for any reason returns null in some conditions (I can't see anything about that in documentation) then write it down because it's something that, likely, a future reader won't know.

Again you're returning List<T> but IEnumerable<T> (according to usage I see in the other functions) will simplify your code (also, initial value of portNameList is used only for error conditions and discarded in the normal code path). Note that using an enumeration you can skip all those ToList().

Something like:

private static IEnumerable<string> GetPortNames()
{
    try
    {
        // When we do not have XYZ privilege then
        // SerialPort.GetPortNames() returns null instead of an empty array.
        return SerialPort.GetPortNames() ?? Enumerable.Empty<string>();
    }
    catch (System.ComponentModel.Win32Exception w32Ex)
    {
       // ...
    }

    return Enumerable.Empty<string>();
}

IMO usage of Error it's somehow weird, it's named error but it's actually a logger.

In GetPorts() I see only two things: you're mixing this logic with UI (and you should avoid this because it makes unit testing almost impossible) and a somehow prolix syntax:

internal static IEnumerable<Port> GetPorts()
    => GetPortNames().Select(portName => new Port(portName));

Port.cs

Is there any reason this class is not sealed?

Remove that public field, you're exposing an implementation detail! If you really really need to make the underlying SerialPort object available then use a read-only property (COMPort should follow common .NET naming guidelines and be renamed to ComPort):

public SerialPort ComPort { get; }

Note that now it's also read-only.

In ctor you have too many magic numbers, why don't you move them to private const fields? Even better: you may introduce a PortSettings entity (made only by properties) which optionally is passed to ctor to create a Port with different settings (but do it only if/when it makes sense).

In ReadWeight() you do something unusual: using (COMPort) will dispose the port - you allocated elsewhere - after its use. It might be that, because of an implementation detail, all the disposed resources are re-allocated when you call Open() (making the call to Dispose() redundant if you call Close()) but it's...an implementation detail. In general you should call Dispose() when you're done with that object. More than that: holding a reference to an object which implements IDisposable forces your class to implement IDisposable (and you're not doing that). Again you should avoid magic numbers, that 20 might well be a private const int NumberOfRetriesWhenReading (or something like that). More fundamental question: given this usage pattern why do you create the port in ctor? Can't you simply create a new one each time you call ReadWeight()?

In ConvertStringToDecimal() you should specify a culture for Decimal.TryParse() (probably CultureInfo.InvariantCulture), I doubt you want to parse numbers according to user locale.

In Open() you're catching InvalidOperationException, just check if port is open or not (and protect your code with some synchronisation mechanism if it's supposed to be multi-thread). More than that: if instead of swallowing exceptions you let them bubble up in the stack then you can simplify your code:

public decimal ReadWeight(bool multiScale)
{
    Open();

    try
    {
        for (int retry=0; retry < NumberOfRetriesWhenReading; ++retry)
        {
            var scaleStringData = ReadData(multiScale);
            var scaleDecimal = ConvertStringToDecimal(scaleStringData);

            if (scaleDecimal != ErrorValue)
                return scaleDecimal;
        }
    }
    finally
    {
        Close();
    }
}

And:

private void Open()
{
    try
    {
        if (!ComPort.IsOpen)
            ComPort.Open();
    }
    catch (ArgumentException e)
    {
        // ...
        throw;
    }
    catch (UnauthorizedAccessException e)
    {
        // ...
        throw;
    }
    catch (IOException e)
    {
        // ...
        throw;
    }
}

Note that I'm catching specific exception that SerialPort.Open() may throw and I'm then re-throwing them. Let the caller deal with this error! Here you just log and then give the responsibility to the caller to repeatedly check if this function succeeded or not (reading SerialPort.IsOpen). How can we change our code to make it easier? Let's introduce a PortCommunicationException (pick a better name...) and use it where appropriate. ReadWeight() may decide to swallow errors (I wouldn't) or give a chance to the caller to do what it's right. Let's see the option to swallow exceptions (simplified code):

public decimal ReadWeight(bool multiScale)
{
    try
    {
         Open();

         try
         {
             // ...
         }
         finally
         {
             Close();
         }
    }
    catch (PortCommunicationException e)
    {
        // ...
    }
}

That try/finally really makes me feel we should re-create the port each time. If you decide to go this way then it may look like this:

public decimal ReadWeight(bool multiScale)
{
    try
    {
         using (var serialPort = new SerialPort(...))
         {
             serialPort.Open();

             // ...
         }
    }
    catch (IOException e)
    {
        // ...
    }
    // And all the other possible exceptions
}

More or less the same reasoning applies to all the other functions in this class then I won't repeat myself. Just a note about SerialPort.Close(): you should give a chance to the port to be closed correctly, Open() may fail if latest Close() has been called not long time before. For this I usually use a retry pattern:

for (int retry=0; retry < NumberOfRetries; ++retry)
{
    try
    {
        ComPort.Open();
        return;
    }
    catch (ExceptionThrownWhenPortIsStillClosing)
    {
        Thread.Sleep(DelayBetweenRetries);
    }
}

Change Thread.Sleep() with Task.Delay() is you're using this inside an async function.

One last word about ProcessMultiPortData(). In the exception handler you wrote: "This happens so often that it's probably not worth logging it". I see two things here:

• You're catching Exception. Assuming that OutOfMemoryException has been thrown then do you want to ignore it? I'd say no then be specific.
• If it's so common then probably it's not an exception. Check your inputs, C# is not Python.

You're asking about delegates and events. Your code now is inherently single-threaded then you probably don't gain anything to introduce callbacks here and there, events may be useful in an asynchronous scenario (for example if you start a read-and-forget request or if you have a background thread which read data).

---

In general the big problem of this code is the way you handle exceptions. Of course there are exceptions (pun intended) but generally speaking they should be used for exceptional conditions, not to control the normal program flow. Debugging, readability and performance will all pay the price for an inappropriate usage.