Handling incoming socket data 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 &lt; 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 &lt; Content.Length &amp;&amp; packetPosition &lt; 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);
                    }
                }
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++] 
});