Message Based Communication Design

Question

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}{ControllerId}{_}{ControllerMessageKeyword}{=Answers}(if any){;}
 
Extension Command: {@}{ExtensionId}{XX}{_}{ExtensionMessageKeyword}{=Parameters}(if any){;}
Extension Acknowledgment: {@}{ExtensionId}{XX}{_}{ExtensionMessageKeyword}{=Parameters}(if any){;}

Controller Instead to provide a dummy interface with the command to the end users, I decided to gather messages and acknowledgment into things what really the user needs through properties, it's more natural as it really represents the Unit:

- Property (Category) Power Supply => Enabled { get; set; } / CurrentMax { set; get; } / Current { get; } / Voltage { get; set; }
- Property (category) Information => SubCategory Identifiers { get; } / SubCategory OperatingTimes { get; } / etc.
- Method On() // Turn On...
- Method Off() // Turn Off...
- Method Lock() // Remote Control Only
- Method Unlock() // Allow Manual Mode
- ReadOnlyKeyedCollection Controllers { get; }
- Etc.
 

Lock() / Unlock() methods could be gathered as a Boolean Property Locked { get; set; } The same for On() / Off() methods into Powered / On { get; set; } or whatsoever.

The fact is that we are making new products, some of them are based on the previous one: like a Controller which is out of the unit for the customers who do not need a big power supply.

For some reasons a Message Keyword does not necessarily match a Command Implementation in the assembly, sometimes the possibilities for one keywords are simply too many (and can lead to have a lot of Nullables as properties of the message), therefore when it's possible a keyword can be used in different command implementations when the concerns are properly separated.

Message => Object
- Acknowledgment (Token Reception: Start / Stop) => Message
- AcknowledgmentUnit => Acknowledgment
- AcknowledgmentController => Acknowledgment
- AcknowledgmentExtension => Acknowledgment
 
- Command (Token Transmission: Start / Stop) => Message
- CommandUnit => Command
- CommandController => Command
- CommandExtension => Command
 

And here is start my real problems, some might think that the context above is not really useful but I believe that knowing the context helps to have proper answers.

I have several problems but I'm not really sure that they can be solved.

• Overuse of generics, and some with "circular" definitions.

• I have some virtual calls in base Constructors.

• and some generics lead for some convenience stuff to generate automatically the properties give in generics via a private setter and reflection, then only the public getter is available with public modifier.

• A lack of interface use, mostly relying on abstracts.

Sometimes C# is not powerful enough to express all the constraints I would like to set and avoid any mistakes for further maintenance and improving this assembly.

I'm gonna paste only the parts presenting issues and introduce the design flaws:

Let's start with Message Class, with the virtual calls in the constructor:

  public abstract class Message<TDeviceId, TDeviceMessageKeyword>
            where TDeviceId: struct, IComparable, IFormattable, IConvertible // Enum "Constraint"
            where TDeviceMessageKeyword: struct, IComparable, IFormattable, IConvertible // Enum "Constraint"
        {
            internal abstract String Header { get; }
            internal abstract String Content { get; }
    
            public abstract TDeviceId Id { get; }
            public abstract TDeviceMessageKeyword MessageKeyword { get; }
    
            private const String _separatorIdKeyword = "_";
            protected String SeparatorIdKeyword
            {
                get
                {
                    return Message<TDeviceId, TDeviceMessageKeyword>._separatorIdKeyword;
                }
            }
    
            private const String _separatorHeaderContent = "=";
            protected String SeparatorHeaderContent
            {
                get
                {
                    return Message<TDeviceId, TDeviceMessageKeyword>._separatorHeaderContent;
                }
            }
    
            private const String _flagStop = ";";
            protected virtual String FlagStop
            {
                get
                {
                    return Message<TDeviceId, TDeviceMessageKeyword>._flagStop;
                }
            }
    
            protected abstract String FlagStart { get; }
    
            public abstract CommunicantPacket Packet { get; }
        }

I'm a little bit embarrassed cause I could use interface to avoid with this kind of "virtual calls in the constructor" warnings but that's not really the purpose of an interface.

  public abstract class Command<TDeviceId, TDeviceMessageKeyword> : Message<TDeviceId, TDeviceMessageKeyword>
            where TDeviceId: struct, IComparable, IFormattable, IConvertible
            where TDeviceMessageKeyword: struct, IComparable, IFormattable, IConvertible
        {
            protected Command(TDeviceId id)
            {
                var idByte = Convert.ToByte(id);
                var idByteStringHex = BitConverter.ToString (new [] { idByte });
    
                this._id = id;
                this._idByteStringHex = idByteStringHex;
            }
    
            private readonly String _idByteStringHex ;
            protected String IdByteStringHex
            {
                get
                {
                    return this._idByteStringHex;
                }
            }
    
            private const String _flagStart = "@";
            protected override sealed String FlagStart 
            {
                get 
                {
                    return Command<TDeviceId, TDeviceMessageKeyword>._flagStart;
                }
            }
    
            private readonly TDeviceId _id;
            public override TDeviceId Id
            {
                get 
                {
                    return this._id;
                }
            }
    
            private CommunicantPacket _packet;
            public sealed override CommunicantPacket Packet 
            {
                get 
                {
                    if (this._packet == null) 
                    {
                        // Analysis disable once ConvertIfStatementToConditionalTernaryExpression
                        if (String.IsNullOrEmpty (this.Content)) 
                        {
                            this._packet = new CommunicantPacket (this.FlagStart + this.Header + this.FlagStop);
                        } 
                        else 
                        {
                            this._packet = new CommunicantPacket (this.FlagStart + this.Header + this.SeparatorHeaderContent + this.Content + this.FlagStop);
                        }
                    }
    
                    return this._packet;
                }
            }
        }    
    
        public abstract class Command<TDeviceId, TDeviceKeyword, TAcknowledgment> : Command<TDeviceId, TDeviceKeyword>
            where TDeviceId: struct, IComparable, IFormattable, IConvertible
            where TDeviceKeyword: struct, IComparable, IFormattable, IConvertible
            where TAcknowledgment : Acknowledgment<TDeviceId, TDeviceKeyword>
        {
            protected Command(TDeviceId id)
                : base(id)
            {
            }
    
            // TODO: Optimization... Caching and stuff, using Expression Trees
            internal TAcknowledgment GetAcknowledgment (CommunicantPacket communicantPacket)
            {
                try 
                {
                    var typeAcknowledgment = typeof(TAcknowledgment);
                    const BindingFlags bindingFlags = BindingFlags.Instance | BindingFlags.NonPublic;
                    var arguments = new Object[] { communicantPacket, this };
                    var acknowledgment = Activator.CreateInstance (typeAcknowledgment, bindingFlags, null, arguments, null) as TAcknowledgment;
    
                    return acknowledgment;    
                } 
                catch (Exception exception) 
                {
                    if (exception.InnerException != null)
                    {
                        throw exception.InnerException;
                    }
                    else
                    {
                        throw;
                    }
                }
            }
        }
    
    
    public abstract class Acknowledgment<TDeviceId, TDeviceMessageKeyword> : Message<TDeviceId, TDeviceMessageKeyword>
            where TDeviceId: struct, IComparable, IFormattable, IConvertible
            where TDeviceMessageKeyword: struct, IComparable, IFormattable, IConvertible
        {
            protected Acknowledgment(CommunicantPacket acknowledgmentPacket)
                : base()
            {
                if (String.IsNullOrEmpty(acknowledgmentPacket.Data.String))
                {
                    var message = @"The argument ""acknowledgmentPacket"" cannot be null or empty.";
                    throw new ArgumentNullException ("acknowledgmentPacket", message);
                }
                else
                {
                    const String pattern = @"^(?<FlagStart>{0})(?<Header>{1})((?<SeparatorHeaderContent>{2})(?<Content>{3}))?(?<FlagStop>{4})";
                    var patternApplied = String.Format (pattern, this.FlagStart, RegexProxy.AnyOnceOrMoreButEqual, this.SeparatorHeaderContent, RegexProxy.AnyOnceOrMore, this.FlagStop);
    
                    var regex = new Regex (patternApplied);
    
                    var match = regex.Match (acknowledgmentPacket.Data.String);
    
                    if (match.Success)
                    {
                        this._header = match.Groups ["Header"].Value;
                        this._content = match.Groups ["Content"].Value;
                        this._packet = acknowledgmentPacket;
                    }
                    else
                    {
                        throw new FormatException(acknowledgmentPacket.Data.String);
                    }
                }
            }
    
            private readonly String _header;
            internal override String Header 
            {
                get 
                {
                    return this._header;
                }
            }
    
            private readonly String _content;
            internal override String Content 
            {
                get 
                {
                    return this._content;
                }
            }
    
            private const String _flagStart = "#";
            protected override sealed String FlagStart 
            {
                get 
                {
                    return Acknowledgment<TDeviceId, TDeviceMessageKeyword>._flagStart;
                }
            }
        
            private readonly CommunicantPacket _packet;
            public sealed override CommunicantPacket Packet
            {
                get
                {
                    return this._packet;
                }
            }
        }
    
        public abstract class Acknowledgment<TDeviceId, TDeviceKeyword, TCommand> : Acknowledgment<TDeviceId, TDeviceKeyword>
            where TDeviceId: struct, IComparable, IFormattable, IConvertible
            where TDeviceKeyword: struct, IComparable, IFormattable, IConvertible
            where TCommand : Command<TDeviceId, TDeviceKeyword>
        {
            private void VirtualCallProcessContent (TCommand relatedCommand, String content)
            {
                this.ProcessContent (relatedCommand, content);
            }
    
            protected Acknowledgment(CommunicantPacket acknowledgmentPacket, TCommand relatedCommand)
                : base(acknowledgmentPacket)
            {
                if (this.Header != relatedCommand.Header)
                {
                    String message = String.Format (@"The Command and Acknowledgment Headers, respectively @""{0}"" and ""{1}"", are not matching.", this.Header, relatedCommand.Header);
                    throw new FormatException (message);
                }
                else
                {
                    this._id = relatedCommand.Id;
                    this._keyword = relatedCommand.MessageKeyword;
                    this._relatedCommand = relatedCommand;
    
                    this.VirtualCallProcessContent (relatedCommand, this.Content);
                }
            }
    
            protected abstract void ProcessContent (TCommand relatedCommand, String content);
    
            private readonly TDeviceKeyword _keyword;
            public override TDeviceKeyword MessageKeyword 
            {
                get 
                {
                    return this._keyword;
                }
            }
    
            private readonly TDeviceId _id;
            public override TDeviceId Id 
            {
                get 
                {
                    return this._id;
                }
            }
    
            private readonly TCommand _relatedCommand;
            public TCommand RelatedCommand
            {
                get
                {
                    return this._relatedCommand;
                }
            }
        }

Then we have the internal TAcknowledgment GetAcknowledgment (CommunicantPacket communicantPacket) done with Reflection to avoid the user to have to redefine it in every inherited implementions, apart that this constructor calls could be cached to run nearly as fast as compiled instructions (thanks to compiled expressions), I feel like something smells here.

Also an abstract method that is also called in the constructor... which btw does not help but enforces the fact this method needs to be implemented for each command inherited.

This is mostly all about the message design flaws...

The other big design flaws are related to the classes representing the physical Devices.

   public abstract class Device : Disposer, IDevice
    {
        protected Device(Byte id)
        {
            this._id = id;
        }
    
        private readonly Byte _id;
    
        #region IDevice Implementation
        Byte IDevice.Id 
        {
            get 
            {
                return this._id;
            }
        }
        #endregion
    }
 
    // TODO: Check possible improvements using Laziness...
        public abstract class Device<TDeviceId, TDeviceInformation> : Device
            where TDeviceId : IComparable, IConvertible, IFormattable
            where TDeviceInformation : IDeviceInformation
    {
        protected Device(TDeviceId id)
            : base(Convert.ToByte(id))
        {
            this._id = id;
 
            var typeDeviceInformation = typeof(TDeviceInformation);
 
            const BindingFlags bindingFlags = BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance;
 
            this._information = (TDeviceInformation)Activator.CreateInstance (typeDeviceInformation, bindingFlags, null, new Object[] { this }, null);
        }
 
        private readonly TDeviceId _id;
        public TDeviceId Id
        {
            get
            {
                return this._id;
            }
        }
    
        private readonly TDeviceInformation _information;
        public TDeviceInformation Information
        {
            get
            {
                return this._information;
            }
        }
    }
 

    public interface IDeviceCategory
    {
    }
 
    public abstract class DeviceCategory<TDevice> 
        where TDevice : class, IDevice
    {
        internal DeviceCategory (TDevice parentDevice)
        {
 
        }
 
        private readonly TDevice _parentDevice;
        internal TDevice ParentDevice
        {
            get
            {
                return this._parentDevice;
            }
        }
    }
 
    public interface IDeviceInformation : IDeviceCategory
    {
    }
 
    public abstract class DeviceInformation<TDevice> : DeviceCategory<TDevice>, IDeviceInformation
        where TDevice : class, IDevice
    {
        internal DeviceInformation (TDevice parentDevice)
            : base (parentDevice)
        {
 
        }
    }
 
    public abstract class DeviceCategorySub<TParentDeviceCategory>
        where TParentDeviceCategory : class, IDeviceCategory
    {
        internal DeviceCategorySub (TParentDeviceCategory parentDeviceCategory)
        {
            this._parentDeviceCategory = parentDeviceCategory;
        }
 
        private readonly TParentDeviceCategory _parentDeviceCategory;
        protected TParentDeviceCategory ParentDeviceCategory
        {
            get
            {
                return this._parentDeviceCategory;
            }
        }
    }

Here I didn't find any proper way to enforce properly constraints so I have those useless interfaces instead, and still it does not enforce constraints enough.

I dunno if there something any other implementations that would make my code more elegant, and at the same time keeping its convenience.

Answer 1

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<TDeviceId, TDeviceMessageKeyword>._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.