Handling incoming socket data

Question

Below I have some code that I use to handle incoming socket data, this is not full thing but its the part I am worried about, is there any way to improve this?

int packetPosition = 0;
 while (packetPosition < packetData.Length)
 {
 try
 {
 int MessageLength = Apple.Encoding.DecodeInt32(new byte[] { packetData[packetPosition++], packetData[packetPosition++], packetData[packetPosition++] });
 uint MessageId = Apple.Encoding.DecodeUInt32(new byte[] { packetData[packetPosition++], packetData[packetPosition++] });
 byte[] Content = new byte[MessageLength - 2];
 for (int i = 0; i < Content.Length && packetPosition < packetData.Length; i++)
 {
 Content[i] = packetData[packetPosition++];
 }
 IncomingPacket incomingPacket = new IncomingPacket(MessageId, Content);
 }
 catch (Exception e)
 {
 _log.Error(e);
 Apple.Game.PlayerManager.DisposePlayer(_playerData.PlayerId);
 }
 }

• IEatBagels: It'd be appreciated to have the full thing. It'd make this much easier to understand and therefore help you! :)
• Mast: Is that your original indentation?
• Konrad Morawski: Naming variables with capital letter is really confusing, and at odds with C# convention (int MessageLength, uint MessageId, byte[] Content), especially since you're not even consistent about it (int packetPosition, IncomingPacket incomingPacket).

Answer 1

Assuming the completely broken indentation is a paste glitch (right?), this bit hits me first:

catch (Exception e)
{
 _log.Error(e);
 Apple.Game.PlayerManager.DisposePlayer(_playerData.PlayerId);
}

If DisposePlayer does what it seems to be doing, then this looks like dangerous code that's begging for ObjectDisposedException to be thrown at another, unrelated point in time.

And if your logging framework ever gets configured to throw exceptions, and _log.Error(e); actually throws (IDK, error writing to log file for example), then DisposePlayer is never going to get called.

That's why disposal of resources should happen in a finally block.

catch (Exception e)
{
 _log.Error(e);
 disposePlayer = true;
}
finally
{
 if (disposePlayer)
 {
 Apple.Game.PlayerManager.DisposePlayer(_playerData.PlayerId);
 }
}

That way you know that that bit of code will run no matter what. I added a little bool disposePlayer local variable here, because a finally block will run whether an exception is caught or not, and you only need to call that method when an exception is effectively thrown.

I'm just not comfortable seeing an object being disposed by someone else than its creator, but you haven't given us enough code to chew on for me to expand on this point.

---

I think these two lines take up too much horizontal space:

int MessageLength = Apple.Encoding.DecodeInt32(new byte[] { packetData[packetPosition++], packetData[packetPosition++], packetData[packetPosition++] });
uint MessageId = Apple.Encoding.DecodeUInt32(new byte[] { packetData[packetPosition++], packetData[packetPosition++] });

I would be better to give it vertical height, so that the reader can more easily see how many times packetPosition is being incremented in the process:

int MessageLength = Apple.Encoding.DecodeInt32(new byte[] 
{ 
 packetData[packetPosition++], 
 packetData[packetPosition++], 
 packetData[packetPosition++] 
});
uint MessageId = Apple.Encoding.DecodeUInt32(new byte[] 
{ 
 packetData[packetPosition++], 
 packetData[packetPosition++] 
});