Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
    }
}
share|improve this question
2  
Is there a reason why are you doing this all by yourself, instead of using a library like Protocol Buffers? –  svick Jan 29 '13 at 21:01
    
@svick If tlum is like me it could be that we have never heard of Protocol Buffers :) Thanks for the link. –  dreza Jan 29 '13 at 21:22
    
@svick never having heard of it is one major reason, and to be enlightened to it is another. Thanks for the link. –  tlum Feb 1 '13 at 4:06
add comment

1 Answer 1

up vote 2 down vote accepted

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

  1. 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?

  2. A couple of inconsistencies in your public property camel casing. I noticed you had a fill1. It should be Fill1 to be consistent.

  3. 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.

  4. 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...

share|improve this answer
    
I think using object in your interfaces is a bad idea, I don't see any reason why they shouldn't be generic. And property with the same name as its type is fine, it's certainly not a reason to rename the type. –  svick Jan 29 '13 at 21:05
    
Sure, could make the interface generic. Probably a good idea although I vaguely remember a problem I had done that track a while back. As for property names same as type. I personally think they should be different. If not the type name then rename the property. –  dreza Jan 29 '13 at 21:08
    
Could you explain why do you think they should be different? If I have a type that represents a color, I think Color is a good name for that. Similarly, if I have a property of something that represents its color, Color is a good name for that. –  svick Jan 29 '13 at 21:12
    
Also, your code would be really confusing if I didn't know what you meant to write. (Generic class that doesn't use its parameter, Serializer class that implements Deserializer interface, etc.) –  svick Jan 29 '13 at 21:14
    
Well I always thought to help with readability where possible to give a different name. No exact reason other than thought it was better practice. However I note that although it might be recommended at times it's not necessarill best practice so will remove from answer. Cheers for that. –  dreza Jan 29 '13 at 21:18
show 2 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.