current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

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);
                    }
                }
share|improve this question
2  
It'd be appreciated to have the full thing. It'd make this much easier to understand and therefore help you! :) – TopinFrassi Oct 29 '15 at 18:57
1  
Is that your original indentation? – Mast Oct 29 '15 at 19:02
1  
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). – Konrad Morawski Oct 29 '15 at 21:20
up vote 2 down vote

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++] 
});
share|improve this answer

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.