Message Based Communication Design

I'm trying to design an API.NET for some communication purposes with Testing Equipment and I cannot figure out a proper way for designing the architecture.

Basically, we have different (physical) products:

-
A Controller to perform measurement on channel inputs (e.g. Current, Voltage, Frequency,
Digital, Relay, etc.) and generate some signals on its outputs (AWG, Voltage, Frequency, Digital, etc.) and some typical automotive buses: CAN, LIN and K-LINE.

-
A Unit, able to deal with the power supply and to embed several controllers.

-
An Extension to perform some temperature measurements and provide additional inputs, it can connected to a Unit or working as standalone alone device.

The fact is the embedded team (who is charge of designing all the dirty stuff over the hardware) gave me the commands and acknowledgments are not really clear, neither user-friendly, and for some legacy reasons they're not gonna change that...

Mainly cause prior to that situation we used to have a software dealing directly with the commands and it was a huge huge huge mess, aka baked noodles. Since I decided to gather all the communication stuff into assembly dedicated to that matter. It allows btw to make instrument drivers (for NI LabVIEW) by wrapping the objects and methods of the assembly.

I'm pretty proud of that because it allows our systems to be used in many many different Software platforms (e.g. Matlab) and in our own software platforms as well. I also managed myself to make our assembly Mono compliant.

The protocol as it has been defined it not that extensible (not necessarily there is a need to make it extensible but I feel sometimes that it could be better).

```
Unit Command: {@}{UnitId}{XX}{_}{UnitMessageKeyword}{=Parameters}(if any){;}
Unit Acknowledgment: {#}{UnitId}{XX}{_}{UnitMessageKeyword}{=Answer}(if any){;}

Controller Command: {@}{ParentUnitId}{ControllerId}{_}{ControllerMessageKeyword}{=Parameters}(if any){;}
Controller Acknowledgment: {#}{ParentUnitId}{

I focused on some smaller issues, not really on the problems you mentioned (I'm not sure those have simple solutions).

private const String _separatorIdKeyword = "_";
protected String SeparatorIdKeyword
{
    get
    {
        return Message._separatorIdKeyword;
    }
}

You don't need to write the type, just return _separatorIdKeyword; will work.

And protected fields should generally be avoided, but I think that protected const fields are okay.

this._id = id;

One of the reasons for the _ prefix on fields is to avoid the need for this in situations like this. So you could drop either the this or the _ prefix.

Though I assume you're just following your corporate coding style and consistency is more important than few saved characters.

var idByte = Convert.ToByte(id);
var idByteStringHex = BitConverter.ToString (new [] { idByte });

this._id = id;
this._idByteStringHex = idByteStringHex;

I don't see why should the IdByteStringHex property be computed eagerly in the constructor. I think it makes more sense to change the property to something like:

protected String IdByteStringHex
{
    get
    {
        return BitConverter.ToString(new [] { Convert.ToByte(id) });
    }
}

This might be less efficient, but I wouldn't worry about that unless I had a good reason (read: measurements).

private readonly String _idByteStringHex ;
protected String IdByteStringHex
{
    get
    {
        return this._idByteStringHex;
    }
}

The readonly constraint is nice, but personally I think it's not worth making the code so much more verbose than:

protected String IdByteStringHex { get; private set; }

// Analysis disable once ConvertIfStatementToConditionalTernaryExpression

I've found that many pieces of code that could be converted to ternary shouldn't, because it makes them less readable. I suggest that you disable this analysis globally, so that you don't have to litter your code with these comments.

catch (Exception exception) 
{
    if (exception.InnerException != null)
    {
        throw exception.InnerException;
    }
    else
    {
        throw;
    }
}

This means that exceptions that are wrapped in other exceptions will lose their stack trace. It's true that the outer exception is often too generic to be of any use, but I think that having the whole stack trace (from the outer exception and the original from the inner exception) is important enough to leave both in.

Also, why is this code here specifically? Are you going to repeat it anyplace where an exception with inner exceptions could occur?

protected Acknowledgment(CommunicantPacket acknowledgmentPacket)
    : base()

You don't need to call the base constructor explicitly here, the parameterless base constructor is called implicitly if you don't specify otherwise.

var message = @"The argument ""acknowledgmentPacket"" cannot be null or empty.";
throw new ArgumentNullException ("acknowledgmentPacket", message);

Why are you writing a custom message here? Because it's “null or empty” and not just “null”? I think any developer will immediately figure that out from the code, you don't need to spend effort and code to explain that.

var regex = new Regex (patternApplied);

var match = regex.Match (acknowledgmentPacket.Data.String);

If you're not going to reuse the Regex object, you can use the static version of the Regex.Match() method instead.

StackExchange Code Review Q#55144, answer score: 3