Problem
I was interested in feedback on this architecture.
Objective: serialize / deserialize a network application protocol stream.
Approach: Create a class for each layer/object which exposes attributes as well as encapsulating the stream reader/writer.
If instantiated for serialization the object is instantiated with the appropriate attributes, then a serialize method can be invoked with a BinaryWriter argument that results in the object, and sub-objects, serializing themselves to the stream.
Conversely, when instantiated for deserialization the root object is instantiated with a BinaryReader in the constructor. The root object deserializes the stream by reading its attributes from the stream and instantiating instances of any child layer/object(s).
Below is a working simple example of a single layer. It does not currently have the BinaryWriter method implemented, only the reader.
Good idea? Typical? Bad idea? Are there other common architectures or better design patterns?… that of course is before we even get to code quality.
namespace Sony.Protocol.Hdcp
{
public class HdcpGammaData
{
private byte _fill1 = 0x00;
private byte _fill2 = 0x00;
private Table _table;
private Color _color;
private byte _fill3 = 0x00;
private byte _offset;
private byte _numberOfPoints = 0x10;
private List<HdcpGammaPoint> HdcpGammaPoints;
public HdcpGammaData(Table table, Color color, byte offset, List<HdcpGammaPoint> hdcpGammaPoints)
{
Table = table;
Color = color;
Offset = offset;
if (hdcpGammaPoints.Count != _numberOfPoints)
{
throw new System.ArgumentException("hdcpGammaPoints must contain 0x10 points");
}
HdcpGammaPoints = hdcpGammaPoints;
}
public HdcpGammaData(BinaryReader reader)
{
HdcpGammaPoints = new List<HdcpGammaPoint>();
fill1 = reader.ReadByte();
fill2 = reader.ReadByte();
Table = (Table)reader.ReadByte();
Color = (Color)reader.ReadByte();
fill3 = reader.ReadByte();
Offset = reader.ReadByte();
NumberOfPoints = reader.ReadByte();
for (int i = 0; i < _numberOfPoints; i++)
HdcpGammaPoints.Add(new HdcpGammaPoint(reader));
}
public Table Table
{
get { return _table; }
private set
{
if (!Enum.IsDefined(typeof(Table), value))
{
throw new System.ArgumentOutOfRangeException("Table", value, "Table must be between 1 and 3");
}
_table = value;
}
}
public byte fill1
{
get { return _fill1; }
private set { _fill1 = value; }
}
public Color Color
{
get { return _color; }
private set
{
if (!Enum.IsDefined(typeof(Color), value))
{
throw new System.ArgumentOutOfRangeException("Color", value, "Color must be between 0 and 2");
}
_color = value;
}
}
public byte fill2
{
get { return _fill2; }
private set { _fill2 = value; }
}
public byte fill3
{
get { return _fill3; }
private set { _fill3 = value; }
}
public byte Offset
{
get { return _offset; }
private set
{
if (_offset != 0x00 && _offset != 0x10)
{
throw new System.ArgumentOutOfRangeException("Offset", value, "Offset must be 0x00 or 0x10");
}
_offset = value;
}
}
public byte NumberOfPoints
{
get { return _numberOfPoints; }
private set
{
if (_numberOfPoints != 0x10)
{
throw new System.ArgumentOutOfRangeException("NumberOfPoints", value, "NumberOfPoints must be 0x10");
}
_numberOfPoints = value;
}
}
}
public enum Color : byte
{
Red = 0, Green, Blue
}
public enum Table : byte
{
Gamma1 = 0x01, Gamma2, Gamma3
}
}
Solution
I’ not sure about having two constructors that would be used in different contexts i.e. one for serialization and one for serialization. It seems like to much of a constraint on the object before you even begin to use it.
I think I would prefer to having a HdcpGammaDataReader and a HdcpGammaDataWriter (or something along these lines). These could perhaps both implement respectively a IDataReader and IDataWriter class. These classes would then do the serialization as you wish.
Something like this perhaps:
interface IDataSerializer<T>
{
void Seralize(T obj);
}
interface IDataDeserializer<T>
{
T DeSeralize();
}
public partial class HdcpGammaDataSerializer<HdcpGammaData> : IDataDeserializer
{
public HdcpGammaData DeSeralize()
{
// Build my HdcpGameData object in here either
}
}
public partial class HdcpGammaDataDeSerializer<HdcpGammaData> : IDataDeserializer
{
public HdcpGammaData DeSeralize()
{
// deserialize here
}
}
If you wanted to make the Gamma class immutable pass in all the arguments in the constructor. If you wanted to added an extra layer (packet header, CRC etc) around the data object then your serialize could inherit from a base class that first read the surrounding packet data and offloaded the actual payload serialization/serialization as required.
As for the code itself. Just a few minor things
-
I personally try not to make property names the same as their type names although this is just a personal preference. In your case as suggested by svick it is good to leave as is. Here’s a good read on this conundrum I just found after a quick google if intereste. How to avoid using the same identifier for Class Names and Property Names?
-
A couple of inconsistencies in your public property camel casing. I noticed you had a fill1. It should be Fill1 to be consistent.
-
In line with 2. I’m not sure of your application domain but I tend to find properties like fill1, fill2 a bit obscure and don’t tend to suggest what that property is. Perhaps a more meaningful name here to get away from the 1, 2 naming syntax would be appropiate.
-
If you are going to have public properties with backing fields unless you need the private field just use auto-properties.
i.e.
public byte Fill3 { get; private set; }
Alternatively you could do away with the private set altogether and make your field read-only and only expose a public method
i.e.
private readonly byte _fill3; // initialise in constructor
public byte fill3 { get { return _fill3; } }
As you seem to be mixing and matching your assignment in your constructor being fields and properties I would probably just go with the auto-property concept.
Just my 2cents…