if-elseif chain. Is there a better pattern?

Question

I'm working on a TCP Server using .NET and the following code is my first attempt to process the received data (from the socket) to create a packet to deliver. Basically, the packet has a header (7 bytes) where the first 3 bytes are always 0x12 0x34 0x89 and the next 7 bytes are the message length. For example:

0x12 0x34 0x89 0x00 0x00 0x00 0x02 0x20 0x20

The packet then is a 2 length packet and the sent data is 0x20 0x20.

To process this I am using an if-elseif chain, which I don't like. Is there a better pattern to take away that if-elseif chain?

 public void ProcessIncomingData(byte[] buffer, int length)
 {
 for (int i = 0; i < length; i++)
 {
 ProcessByte(buffer[i]);
 }
 }
 private void ProcessByte(byte b)
 {
 if (_status == PacketStatus.Empty && b == FirstHeaderByte)
 {
 _status = PacketStatus.FirstHeaderReceived;
 }
 else if (_status == PacketStatus.FirstHeaderReceived && b == SecondHeaderByte)
 {
 _status = PacketStatus.SecondHeaderReceived;
 }
 else if (_status == PacketStatus.SecondHeaderReceived && b == ThridHeaderByte)
 {
 _status = PacketStatus.ThirdHeaderReceived;
 }
 else if (_status == PacketStatus.ThirdHeaderReceived || _status == PacketStatus.ReceivingPacketLenght)
 {
 const int sizeOfInt32 = sizeof (int);
 const int sizeOfByte = sizeof (byte);
 _packetLength |= b << (sizeOfInt32 - ++_packetLenghtOffset) * sizeOfByte;
 _status = _packetLenghtOffset < 4 ? PacketStatus.ReceivingPacketLenght : PacketStatus.ReceivingData;
 }
 else if (_status == PacketStatus.ReceivingData)
 {
 _packet.Add(b);
 var receivedByteCount = _packet.Count;
 if (receivedByteCount == _packetLength)
 {
 var packetData = new byte[_packet.Count];
 _packet.CopyTo(packetData);
 var receivedPacketHandler = PacketReceived;
 if(receivedPacketHandler != null)
 {
 receivedPacketHandler(this, new PacketReceivedEventArgs{ Packet = packetData });
 }
 ResetControlVariables();
 }
 }
 }

• Leonid: TCP servers already exist, ones written in C are faster. If you want uber speed, the code will have to be messy. If you want clarity, then I would separate the process of scanning the header from the process of scanning the length from reading the actual data.
• Lstor: You have a typo in the code: ThridHeaderByte, and several places you are writing Lenght instead of Length.
• svick: sizeof is meant for use in unsafe code, you shouldn't use it like this.
• lontivero: @Leonid, I know you´re right but this is just a hobby. Thank you for share your recommendation.
• lontivero: @Lstor, thank you, I always make the same mistakes. I´ve fixed them.

Answer 1

Simple pattern: Set up a hash table or map that maps the incoming byte to a function that returns the correct response.

(C++ pseudocode - can be done in C# as well - don't have time now for all the implementation details:)

__statusType responseFunc(_statusType,byte);
map<byte,responseFunc> responseMap;
private void ProcessByte(byte b)
{
 _status= responseMap[_status](_status,b);
}

Although you have some nested conditions, etc, your handler functions can deal with those conditions - but you may have to rethink your design and set up some classes that properly represent and understand your conditions. A long switch case or 'if else if' is invariably the result of poor design. You need to think carefully about your architecture.

Another thing to consider is abstracting your conditions a bit and setting up an enumeration or class hierarchy to represent them. They can then be used as the key for your map or a parameter to your handler functions.

Yes - this is work - but clean design does require some effort. If you don't like long conditional statements (and you shouldn't) put in the effort to clean them up.

You should think along those lines - maps, dictionaries, enumerations, class hierarchies, factories, etc - whenever you run into a situation that seems to require an extended 'if else if..' or 'switch case';

• lontivero: This is not a solution. I have conditions to walk through different states.
• Vector: Your handler functions can deal with those conditions - and you may have to rethink your design and set up some classes that properly represent and understand your conditions. You should not have downvoted - I have been designing systems and writing production code for 20 years: A long switch case or if else if is invariably the result of poor design. You need to think carefully about your architecture.
• Vector: Another thing to consider is abstracting your conditions a bit and setting up an enumeration or class hierarchy to represent them. They can then be used as the key for your map or a parameter to your handler functions. Yes - this is work - but clean design does require some effort. If you don't like long conditional statements (and you shouldn't) put in the effort to clean it up.
• Vector: see edit of answer.
• lontivero: @Mickey, i've fixed the -1 issue and i am grateful for your help and effort. I agree with you on setting up some classes that properly represent and understand my conditions, that's the point. I know very well the pattern that you destails, in fact i wrote about it (geeks.ms/blogs/lontivero/archive/2012/01/30/…). It is just that I think it is not what I am looking for exactly.

Answer 2

There is a bug in that you are not resetting the state when you read an unexpected byte. For example, this byte sequence would be accepted as a valid header by your code:

0x12 0xff 0x34 0xff 0x89

I wonder whether you would get a neater solution if you used a stream:

Stream stream = new MemoryStream(buffer);
ReadHeader(stream);
int length = ReadLength(stream);
byte[] data = new byte[length];
stream.Read(data, 0, length);

Where ReadHeader looks something like this:

byte[] header = new byte[TcpHeader.Length];
stream.Read(header, 0, TcpHeader.Length);
while (!header.equals(TcpHeader)) {
 // move back to 1 byte on from where we last tried to read the header
 stream.Seek(1 - TcpHeader.Length, SeekOrigin.Current);
 stream.Read(header, 0, TcpHeader.Length);
}

And where ReadLength reads 4 bytes and creates an int, taking network byte order into consideration.

Note there are some defects in my code which I have left there to keep it readable:

• The length field is probably an unsigned int, whereas Stream.Read only accepts an int.
• Stream.Read may not read all the bytes you asked for, so a loop is required to fill the buffer.
• We may reach the end of the stream while reading.

(feedback on my first ever answer is very welcome!)

• svick: Your general idea is sound, but I don't think trying to reparse the header at every possible position is a good idea (even if the question indicates that's what's wanted).
• lontivero: @chris, I like your code but, as svick says, reparse the header is something that I don´t want to do. About the defect you mentioned, you're absolutely right. I did a UT to it and fails (Thank you!). +1
• chris: I agree that it seems bad to scan for the header, and I was going to say so in my answer, but on reflection I decided that a TCP server should be tolerant to junk data. I.e., if a valid packet is prefixed with an invalid partial header, I would expect the server to cope with that gracefully. Hence I followed the same logic expressed in the question :)

Answer 3

This answer is just to show the current implementation (proposed by Mikey).

 public PacketHandler()
 {
 _packet = new List<byte>();
 _handlers = new Dictionary<PacketStatus, Action<byte>>{
 { PacketStatus.Empty, b => Expect(FirstHeaderByte, b, PacketStatus.FirstHeaderReceived) },
 { PacketStatus.FirstHeaderReceived, b => Expect(SecondHeaderByte, b, PacketStatus.SecondHeaderReceived) },
 { PacketStatus.SecondHeaderReceived,b => Expect(ThirdHeaderByte, b, PacketStatus.ThirdHeaderReceived)},
 { PacketStatus.ThirdHeaderReceived, AcceptPacketLength},
 { PacketStatus.ReceivingPacketLength, AcceptPacketLength},
 { PacketStatus.ReceivingData, AcceptData},
 { PacketStatus.Unsynchronized, b => Expect(FirstHeaderByte, b, PacketStatus.FirstHeaderReceived) }
 };
 }
 public void ProcessIncomingData(byte[] buffer, int length)
 {
 for (_currentByteIndex = 0; _currentByteIndex < length; _currentByteIndex++)
 {
 var currentByte = buffer[_currentByteIndex];
 var handler = _handlers[_status];
 handler(currentByte);
 }
 }

Answer 4

I come up with a solution using bit mask to detect the header. I think using Dictionary looks more maintainable anyway.

 private int _expectedBytesLength;
 private int _consumedBytesLength;
 private readonly byte[] _packetMask = new byte[] { 0x12, 0x34, 0x89, 0xFF, 0xFF, 0xFF, 0xFF };
 private byte[] _packetHeaderBuffer = new byte[7]; // should be allocated with _packetMask.Length in constructor
 private List<byte> _packet = new List<byte>();
 public void ProcessByte(byte b) {
 if (_expectedBytesLength > 0) {
 if (_expectedBytesLength > _consumedBytesLength) {
 _packet.Add (b);
 _consumedBytesLength++;
 }
 if (_consumedBytesLength >= _expectedBytesLength) {
 var packetData = new byte[_packet.Count];
 _packet.CopyTo (packetData);
 Console.WriteLine (packetData.Length);
 // raise events etc.
 _expectedBytesLength = 0;
 _consumedBytesLength = 0;
 _packet.Clear ();
 }
 } 
 else {
 bool inState = (b & _packetMask[_consumedBytesLength]) == b;
 if (inState) {
 _packetHeaderBuffer[_consumedBytesLength] = b;
 _consumedBytesLength++;
 if (_consumedBytesLength >= _packetMask.Length) {
 int length = 0;
 for (int i = 3; i < 7; i++) {
 length <<= 1;
 length += _packetHeaderBuffer[i];
 }
 _expectedBytesLength = length + _packetHeaderBuffer.Length;
 }
 }
 }
 }