Player Packet System

Question

I have recently coded a system where an instance of class Player is created for each connected socket, once the player class has been created, in its constructor the Socket that connected starts receiving data from the client side.

I won't be including the client side code, just the server side for this question. Once the socket has started receiving, it started receiving bits of data, each message contains a unique packet id which then it finds out the packet identifier with some code, which I have posted below.

What am I asking?
How can I improve this code in any way, any suggestions?

Player.cs:

namespace Hariak_Emulator.Emulator.Base.Game.Habbo.Players.Players
{
    using System;
    using System.Net.Sockets;
    using System.Text;
    using NLog;

    internal sealed class Player : IDisposable
    {
        private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();

        private readonly Socket _socket;
        private readonly PlayerPacketHandler _packetHandler;
        private readonly byte[] _buffer;

        internal Player(Socket socket)
        {
            _socket = socket;
            _packetHandler = new PlayerPacketHandler(this);
            _buffer = new byte[8000];

            StartReceive();
        }

        private void StartReceive()
        {
            try
            {
                _socket.BeginReceive(_buffer, 0, _buffer.Length, SocketFlags.None, OnDataReceived, _socket);
            }
            catch (SocketException)
            {
                Dispose();
            }
        }

        private void OnDataReceived(IAsyncResult iAsyncResult)
        {
            try
            {
                var bytesReceived = _socket.EndReceive(iAsyncResult);

                if (bytesReceived == 0)
                {
                    Dispose();
                    return;
                }

                var packet = new byte[bytesReceived];
                Array.Copy(_buffer, packet, bytesReceived);
                _packetHandler.ProcessPacketData(packet);
            }
            catch
            {
                Dispose();
            }
            finally
            {
                try
                {
                    _socket.BeginReceive(_buffer, 0, _buffer.Length, SocketFlags.None, OnDataReceived, _socket);
                }
                catch
                {
                    Dispose();
                }
            }
        }

        public void SendString(string data)
        {
            SendData(Encoding.UTF8.GetBytes(data));
        }

        private void SendData(byte[] data)
        {
            try
            {
                _socket.BeginSend(data, 0, data.Length, 0, OnSend, null);
            }
            catch (SocketException socketException)
            {
                Logger.Error("Error sending message to socket: " + socketException.Message);
                Logger.Error(socketException);
                Dispose();
            }
        }

        private void OnSend(IAsyncResult asyncResult)
        {
            try
            {
                if (_socket == null)
                {
                    return;
                }

                _socket.EndSend(asyncResult);
            }
            catch (SocketException)
            {
                Dispose();
            }
        }

        public void Dispose()
        {
            Dispose(true);
        }

        private void Dispose(bool disposing)
        {
            if (!disposing)
            {
                return;
            }

            //todo: properly dispose
        }
    }
}

PlayerPacketHandler.cs:

namespace Hariak_Emulator.Emulator.Base.Game.Habbo.Players.Players
{
    using System;
    using System.IO;
    using System.Net.Sockets;
    using System.Text;
    using NLog;
    using Utilities;

    internal sealed class PlayerPacketHandler
    {
        private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();

        private readonly Player _player;
        private readonly byte[] _buffer;

        private byte[] _dataReceived;
        private bool _halfDataRecieved;
        private bool _decryptedData;

        internal PlayerPacketHandler(Player player)
        {
            _player = player;
            _buffer = new byte[8000];
        }

        internal void ProcessPacketData(byte[] receivedData)
        {
            var receivedDataStr = Encoding.Default.GetString(receivedData);
            var receivedDataPacketId = receivedData[0];

            if (receivedDataPacketId == 67)
            {
                return;
            }

            if (receivedDataPacketId == 60)
            {
                const string crossDomainPolicy = "<?xml version=\"1.0\"?>\r\n"
                    +
                    "<!DOCTYPE cross-domain-policy SYSTEM \"/xml/dtds/cross-domain-policy.dtd\">\r\n"
                    + "<cross-domain-policy>\r\n"
                    +
                    "<site-control permitted-cross-domain-policies=\"master-only\"/>\r\n"
                    + "<allow-access-from domain=\"*\" to-ports=\"*\" />\r\n"
                    + "</cross-domain-policy>\x0";

                _player.SendString(crossDomainPolicy);
            }
            else
            {
                using (var reader = new BinaryReader(new MemoryStream(receivedData)))
                {
                    if (receivedData.Length < 4)
                    {
                        return;
                    }

                    var packetLength = PacketUtilities.DecodeInt32(reader.ReadBytes(4));

                    if (reader.BaseStream.Length - 4 < packetLength)
                    {
                        _dataReceived = receivedData;
                        _halfDataRecieved = true;
                        return;
                    }

                    if (packetLength < 0 || packetLength > 5120)
                    {
                        return;
                    }

                    var packetBytes = reader.ReadBytes(packetLength);

                    using (var binaryReader = new BinaryReader(new MemoryStream(packetBytes)))
                    {
                        var packetHeader = PacketUtilities.DecodeInt16(binaryReader.ReadBytes(2));
                        var packetBodyBytes = new byte[packetBytes.Length - 2];

                        Buffer.BlockCopy(packetBytes, 2, packetBodyBytes, 0, packetBytes.Length - 2);
                        Hariak.HariakServer.GameManager.PacketManager.ProcessPacket(_player, packetHeader);

                        _decryptedData = false;
                    }

                    if (reader.BaseStream.Length - 4 <= packetLength)
                    {
                        return;
                    }

                    var extra = new byte[reader.BaseStream.Length - reader.BaseStream.Position];
                    Buffer.BlockCopy(receivedData, (int)reader.BaseStream.Position, extra, 0, (int)(reader.BaseStream.Length - reader.BaseStream.Position));

                    _decryptedData = true;
                    ProcessPacketData(extra);
                }
            }
        }
    }
}

Hariak.HariakServer.GameManager.PacketManager:

namespace Hariak_Emulator.Emulator.Core.Habbo.Packets
{
    using System;
    using System.Collections.Generic;
    using Base.Game.Habbo.Players.Players;
    using Events;
    using NLog;

    internal class PacketManager
    {
        private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();

        private readonly Dictionary<int, KeyValuePair<string, IPacketEvent>> _packets;

        public PacketManager()
        {
            _packets = new Dictionary<int, KeyValuePair<string, IPacketEvent>>();
        }

        public void LoadPackets()
        {
            // For this example I only added 1 packet, there will be over 100 on the actual code.
            _packets.Add(1, new KeyValuePair<string, IPacketEvent>("MyPacketEventsNameHere", new MyPacketEvent()));
        }

        public void ProcessPacket(Player player, int packetId)
        {
            KeyValuePair<string, IPacketEvent> packet;

            if (_packets.TryGetValue(packetId, out packet))
            {
                Logger.Debug("Player has handled packet '" + packet.Key + "' => " + packetId + "");
                packet.Value.ProcessPacket(player);
            }
            else
            {
                Logger.Warn("Missing packet => " + packetId + "");
            }
        }
    }
}

IPacketEvent:

namespace Hariak_Emulator.Emulator.Core.Habbo.Packets
{
    internal interface IPacketEvent
    {
        void ProcessPacket(Player player);
    }
}

MyPacketEvent.cs:

namespace Hariak_Emulator.Emulator.Core.Habbo.Packets.Events
{
    internal class MyPacketEvent: IPacketEvent
    {
        public void ProcessPacket(Player player)
        {
            // process packet here?
        }
    }
}

PacketUtilities:

namespace Hariak_Emulator.Emulator.Utilities
{
    internal static class PacketUtilities
    {
        internal static int DecodeInt32(byte[] bytes)
        {
            if ((bytes[0] | bytes[1] | bytes[2] | bytes[3]) < 0)
            {
                return -1;
            }

            return (bytes[0] << 0x18) + (bytes[1] << 0x10) + (bytes[2] << 8) + bytes[3];
        }

        internal static int DecodeInt16(byte[] bytes)
        {
            if ((bytes[0] | bytes[1]) < 0)
            {
                return -1;
            }
            return (bytes[0] << 8) + bytes[1];
        }
    }
}

Answer 1

1. You have way too many Dispose calls in Player class, half of which smell. This for example:

           catch
           {
               Dispose();
           }
           finally
           {
               try
               {
                   _socket.BeginReceive(_buffer, 0, _buffer.Length, SocketFlags.None, OnDataReceived, _socket);
               }
               catch
               {
                   Dispose();
               }
           }
   

   pretty much guarantees, that you will work with disposed state of your object in your finally block if you catch an exception earlier. Dispose should be called by external code when it is done using your object. Not by object itself when you assume it is no longer needed. Also, your Dispose method is incomplete, both in terms of following dispose pattern and in terms of actual implementation, which raises all sorts of red flags. If you don't know how to properly implement dispose method - don't implement it at all until you do. If you do know - then do so before asking for review.

2. "Player" is not the best name to describe what the class does. I think Connection (or PlayerConnection) is better.

3. Callback-based APIs are somewhat obsolete. If you are using modern version of C# you might want to use asycn-based API instead. Look up Socket.____Async methods on MSDN

4. new PlayerPacketHandler(this) is the kind of tight coupling you might want to avoid. I do not know what is the best way to refactor your code, since I have no context, but the most straightforward way is to add return value to ProcessPacketData method:

   var packet = new byte[bytesReceived];
   Array.Copy(_buffer, packet, bytesReceived);
   
   byte[] response = _packetHandler.ProcessPacketData(packet);
   if (response != null)
   {
       Send(response);
   }
   

   this way PlayerPacketHandler no longer needs a reference to Player and can be used and, more importantly, unit-tested separately from it.

5. As you can see ProcessPacketData method is already pretty large, and you only have 3 types of packets so far. If more is on the way, then you should consider abstracting parsing logic into separate components (on one parser per packet type basis, for example).

6. Buffer size probably shouldn't be hard-coded in multiple places. Instead it should come from configuration of sorts, so it is easy to tweak if needed.
7. Static dependencies are evil. First couple of times I looked through your code I completely missed Hariak.HariakServer.GameManager.PacketManager static reference hidden deep inside your code. And so will any other person reading it. If you need a dependency - inject it in constructor, don't randomly access it anyplace you want. This helps to make your code more readable, more testable and less error-prone.

8. Overall, I think the main improvement you can make is to make your processing logic flat. Your Player calls "handler", which calls "manager", which calls Player again, etc. This callstack is too deep and too complex. Instead lay it out in one place, so the whole process is easy to understand. Here is some pseudo-code:

   //get raw packet from network
   byte[] data = await _connection.Receive();
   //parse raw buffer into strongly typed object
   IPacket packet = _parser.Parse(data);
   //process the data
   IPacket response = _processor.ProcessData(packet);
   if (response != null)
   {
       //send response if any
       _connection.Send(response.ToArray());
   }
   

   The point of above example is that _parser does not call _processor and have no knowledge of it. Same goes for relation between _processor and _connection. This is the kind of loose coupling you should try to achieve.