PlayerConnection class for Gaming Emulator

Question

I have programmed a Gaming Emulator in C# and I have a class to handle all the connection side of things for the player, here we continuously listen for packets (each packet has a ID like a number, from 1 - 300), I just wondered is there any way I could speed it up or make it better performance wise? It seems a bit messy at the moment..

using Faze.Other.Util;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net.Sockets;
using System.Text;
using System.Threading.Tasks;

namespace Faze.Base.Game.Habbo.Players
{
    class PlayerConnection
    {
        private readonly Socket connectionSocket;
        private bool packetsActive;
        private readonly byte[] connectionBuffer;
        private bool isPlayerDisconnecting;

        public PlayerConnection(Socket playerSocket)
        {
            connectionSocket = playerSocket;
            connectionSocket.SendBufferSize = 8000;
            connectionBuffer = new byte[8000];
        }

        public Socket PlayersSocket
        {
            get
            {
                return connectionSocket;
            }
        }

        private void ListenForPackets()
        {
            if (packetsActive)
                return;

            packetsActive = true;

            try
            {
                connectionSocket.BeginReceive(connectionBuffer, 0, connectionBuffer.Length, SocketFlags.None, OnPacketReceived, connectionSocket);
            }
            catch
            {
                CloseSocket();
            }
        }

        private void CloseSocket()
        {
            try
            {
                if (!packetsActive)
                    return;

                packetsActive = false;

                if (SocketConnected(connectionSocket))
                {
                    connectionSocket.Shutdown(SocketShutdown.Both);
                    connectionSocket.Close();
                }

                connectionSocket.Dispose();
            }
            catch
            {
            }
        }

        private bool SocketConnected(Socket socket)
        {
            bool part1 = socket.Poll(1000, SelectMode.SelectRead);
            bool part2 = (socket.Available == 0);

            if (part1 && part2)
                return false;
            else
                return true;
        }

        private void OnPacketReceived(IAsyncResult asyncResult)
        {
            try
            {
                int bytesReceived;

                if (isPlayerDisconnecting)
                    return;

                bytesReceived = connectionSocket.EndReceive(asyncResult);

                if (bytesReceived == 0 /*|| bytesReceived != typeof(int)*/)
                {
                    CloseSocket();
                    return;
                }

                var packet = new byte[bytesReceived];
                Array.Copy(connectionBuffer, packet, bytesReceived);

                if (packet.Length == 0)
                    return;

                if (packet[0] == 60)
                {
                    SendData("<?xml version=\"1.0\"?>\r\n<!DOCTYPE cross-domain-policy SYSTEM \"/xml/dtds/cross-domain-policy.dtd\">\r\n<cross-domain-policy>\r\n<allow-access-from domain=\"*\" to-ports=\"1-31111\" />\r\n</cross-domain-policy>\x0");
                }
                else if (packet[0] != 67)
                {
                    int packetPosition = 0;
                    for (packetPosition = 0; packetPosition < packet.Length;)
                    {
                        int packetLength = ClassHelper.DecodeInt32(new[]
                        {
                            packet[packetPosition++],
                            packet[packetPosition++],
                            packet[packetPosition++],
                            packet[packetPosition++]
                        });

                        if (packetLength < 2 || packetLength > 4096)
                            return;

                        uint packetId = ClassHelper.DecodeUInt32(new[]
                        {
                            packet[packetPosition++],
                            packet[packetPosition++]
                        });

                        byte[] packetContent = new byte[packetLength - 2];

                        for (int i = 0; i < packetContent.Length && packetPosition < packet.Length; i++)
                            packetContent[i] = packet[packetPosition++];

                        // Short packetId is the packet id, todo: handle it.
                    }
                }
            }
            catch
            {
                CloseSocket();
                return;
            }
            finally
            {
                try
                {
                    connectionSocket.BeginReceive(connectionBuffer, 0, connectionBuffer.Length, SocketFlags.None, OnPacketReceived, connectionSocket);
                }
                catch
                {
                    CloseSocket();
                }
            }
        }

        public void CloseConnection()
        {
            if (isPlayerDisconnecting)
                return;

            isPlayerDisconnecting = true;
        }

        public bool PlayerDisconnecting
        {
            get { return isPlayerDisconnecting; }
        }

        private void SendData(string Data)
        {
            SendData(Encoding.UTF8.GetBytes(Data));
        }

        private void SendData(byte[] data)
        {
            string packetData = Encoding.Default.GetString(data);
            connectionSocket.BeginSend(data, 0, data.Length, 0, OnSentDataCallback, null);
        }

        private void OnSentDataCallback(IAsyncResult asyncResult)
        {
            try
            {
                connectionSocket.EndSend(asyncResult);
            }
            catch
            {
                CloseSocket();
            }
        }
    }
}

ClassHelper class:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Faze.Other.Util
{
    public static class ClassHelper
    {
        public static int DecodeInt32(byte[] toDecode)
        {
            if ((((toDecode[0] | toDecode[1]) | toDecode[2]) | toDecode[3]) < 0)
                return -1;

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

        public static uint DecodeUInt32(byte[] toDecode)
        {
            return (uint)DecodeInt32(toDecode);
        }
    }
}

Answer 1

Private functions & fields aren't called/modified within the class:

• ListenForPackets is private and isn't called by anything within the class

• packetsActive is private and isn't written to by anything within the class

(so they can only be accessed through reflection, which is not a normal way for classes to interact)

Note that the private accessibility level in C# is intended to mean that the entry can only be accessed within the same type, i.e., within PlayerConnection only.

The containing class is not intended to have any special access rights at all.

For a demonstration see this code below:

Or try it here: https://ideone.com/zJGFE0

using System;

public class MyClassContainer
{
    private MyClass MyClass_Instance;
    
    public void DemonstrateAccessLevels()
    {
        MyClass_Instance = new MyClass();
        
        MyClass_Instance.SomePublicMethod();
        
        MyClass_Instance.SomeInternalMethod();
        
        // we can't access protected or private methods of MyClass from MyClassContainer
        //
        //error CS0122: `MyClass.SomeProtectedMethod()' is inaccessible due to its protection level
        //MyClass_Instance.SomeProtectedMethod();
        
        //error CS0122: `MyClass.SomePrivateMethod()' is inaccessible due to its protection level
        //MyClass_Instance.SomePrivateMethod();
    }
}

public class MyClass
{
    private void SomePrivateMethod()
    {
        Console.WriteLine("Some Private Method");
    }
    
    protected void SomeProtectedMethod()
    {
        Console.WriteLine("Some Protected Method");
    }
    
    internal void SomeInternalMethod()
    {
        Console.WriteLine("Some Internal Method");
    }
    
    public void SomePublicMethod()
    {
        Console.WriteLine("Some Public Method");
    }
}

public class Test
{
    public static void Main()
    {
        MyClassContainer testContainer = new MyClassContainer();
        
        testContainer.DemonstrateAccessLevels();
    }
}

---

The Accessibility Levels on MSDN article gives you your access rights options in C#, I recommend designing within these access rights and not trying to work around them with reflection.

Please note that "containing class" refers to the class being defined, NOT the class containing an instance of the class we're defining.

• public: Access is not restricted.

• protected: Access is limited to the containing class or types derived from the containing class.

• internal: Access is limited to the current assembly. [i.e., current .exe or .dll]

• protected internal: Access is limited to the current assembly or types derived from the containing class.

• private: Access is limited to the containing type.

It would be clearer to add getters/setters and public methods to access your class's state instead of using reflection to get at private methods. Using reflection to get at private methods is kind of deceptive and people who read your code might not expect code outside of the class to be modifying those fields / calling those methods.

If you put a private method in a class, and never call it within the class, it looks like dead code. Even worse, it breaks encapsulation, if any class can modify your class's private fields, your class doesn't really offer any protection.

Note that in addition to this, invoking methods using reflection can be very slow, for more information see this question: https://stackoverflow.com/questions/10313979/methodinfo-invoke-performance-issue

---

Encapsulation anti-pattern:

The PlayerSocket getter allows other classes to run functions to modify the socket. Your PlayerSocket getter is in my opinion worse than making the player's Socket a public field.

If your intention is to have this class encapsulate your socket, this won't do it, keep in mind external classes will still be able to call stuff like Close() without PlayerConnection even knowing about it, making the CloseSocket() method on PlayerConnection an optional method, and kind of deceptive.

My recommendations for this part of the code depend on what you want to accomplish.

• If you want PlayerConnection to encapsulate the Socket: I would recommend getting rid of the Getter property and add public methods to PlayerConnection that provide access to only the methods on Socket that you want external classes to access (e.g., maybe everything except Socket.Close()). That way you can better control access to the socket.

• If you don't want PlayerConnection to encapsulate the Socket, and you're okay with other classes modifying the Socket: I would recommend possibly moving all of this code into the Player class, if feels too cumbersome to restrict access to the socket this may not need its own distinct class.

If the Player class is already very big, I'd recommend posting a new code review with both the Player class and the PlayerConnection class, with the access modifiers fixed; we'd probably be able to give more advice at that point.

In general, you should almost never have a getter that just returns an entire private reference type field like Socket. Making a private field, then a public getter nullifies most benefits of having that field being private.

Any class that calls the getter can modify that Socket field's data (by calling methods on that returned field). From the perspective of PlayerConnection, those methods called are silently modifying that field, which is supposed to be encapsulated by your class.

Answer 2

Always use braces, you should never omit them as it can lead to bugs.

---

Try to find a way to represent your magic numbers.

if (packet[0] == 60)
else if (packet[0] != 67)
if (packetLength < 2 || packetLength > 4096)
connectionSocket.SendBufferSize = 8000;

There's no telling what those numbers (60 and 67) mean and why we're looking at packet[0] to determine if that's what they are. Create a constant that represents them and compare against that.

const byte someIdentifier = 60;
const byte someIdentifier2 = 67;
const int packetLengthMinimum = 2;
const int packetLengthMaximum = 4096;
const int maxSendBufferSize = 8000;

Obviously replace the names with something more meaningful.

This allows you to, in the future, if technical concerns require/allow it, to change the values or make them properties and configurable, etc.

---

You can reduce almost all of the parenthesis in this method:

    public static int DecodeInt32(byte[] toDecode)
    {
        if ((((toDecode[0] | toDecode[1]) | toDecode[2]) | toDecode[3]) < 0)
            return -1;

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

public static int DecodeInt32(byte[] toDecode)
{
    if (toDecode[0] | toDecode[1] | toDecode[2] | toDecode[3] < 0)
    {
        return -1;
    }

    return (toDecode[0] << 0x18) + (toDecode[1] << 0x10) + (toDecode[2] << 0x08) + (toDecode[3] << 0x00);
}

---

Try to use consistent naming conventions.

Your PlayersSocket uses the private field connectionSocket, which is obviously a different name. Try to make them the same so that if you are bouncing between code when debugging, you don't need it in your head that any outside code referencing PlayersSocket is actually referencing the internal connectionSocket.

---

        connectionSocket.SendBufferSize = 8000;
        connectionBuffer = new byte[8000];

You have 8000 hardcoded twice; do as I said above, make a constant for it, then have connectionBuffer = new byte[connectionSocket.SendBufferSize];.

---

Overall, this seems quite good. It just needs minor tweaks to make it a little easier to read and maintain. Good work!

Answer 3

Beware the leaky abstraction

As has been said by @jrh, having your PlayersSocket returned by a public getter is a flag that your classes encapsulation has a hole in it and might not make sense. Once the socket has been passed into the constructor for PlayerConnection, the instance should take complete responsibility for the Socket. If it allows access to the raw socket, then it can't maintain that level of control. As it stands, there's nothing stopping a client class calling:

playerConnection.PlayersSocket.Close();

Unused Usings

As far as I can tell, you're not using anything from these namespaces:

using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;

If you're not using them, remove them. Anything that doesn't need to be there just adds clutter. It's the same for commented out code. If you don't need it, get rid of it. When you see a line like:

if (bytesReceived == 0 /*|| bytesReceived != typeof(int)*/)

It just makes you wonder why it's been commented out. Should it really be uncommented, is the comment to get it to go down a specific path. If the code had just been removed, it would be easier to read.

DecodeInt32 - exceptions are your friend

This makes no sense to me:

if ((((toDecode[0] | toDecode[1]) | toDecode[2]) | toDecode[3]) < 0)
    return -1;

byte is an unsigned type, it's never going to be < 0. And if you somehow managed to get it to trigger this criteria, would you really want the method to return -1? -1 seems like a fairly expected value when decoding an int32, if you decide that you've got unexpected data and you don't know what to do with it, throw an exception, don't just fake a return value. Faking a value is likely to lead to hidden bugs in the future. Better to throw an InvalidOperationException or ArgumentException and know you have a problem that you need to deal with.

Events and Delegates

Having a separation in concerns between the Player and PlayerConnection makes sense in a lot of scenarios, not least it allows you to maintain the Player state if a player loses a connection and then reconnects through a different Socket. However, for this to work there has to be some way for the Player and the Connection to communicate. One approach might be for the player to register a delegate / event with the connection so that it gets a notification on packet received (or a higher level abstraction). There's no obvious way in your existing code for data to get from the Connection to the Player. This suggests that the abstractions in your code don't entirely make sense.

ListenForPackets

As has been pointed out by @jrh, ListenForPackets is marked as private and doesn't appear to be called from within the class. The method itself is using packetsActive as a flag to indicate if the method should actually do anything or not. This suggests it might be expecting to be called when a BeginReceive has already been called. This suggests that the socket might not be being managed as well as I would expect, however without the calling code it's hard to say.

CloseConnection

This method doesn't really do anything other than setting isPlayerDisconnecting=true. Checking if it's already true beforehand seems somewhat superfluous, you might as well just always set it:

public void CloseConnection()
{
    isPlayerDisconnecting = true;
}

For what it's worth, I'm not entirely sure CloseConnection is the right name for something that doesn't actually close the connection. The flag is used to prevent more received data from being processed, not to trigger a socket closure.

When you're done, you're done

OnPacketsReceived is used to process received information. It wraps the processing in a try/catch/finally block. The finally block triggers another read. This makes sense in scenarios where you're keeping the socket open and you want to instruct it to keep receiving data over and over again. However, it doesn't make sense after the class has been instructed to close the connection. This:

try
{
    int bytesReceived;

    if (isPlayerDisconnecting)
        return;

Should probably be changed to:

if (isPlayerDisconnecting)
    return;
try
{
    int bytesReceived;

As it stands, the method is using a isPlayerDisconnecting to decide that it should ignore any data that has arrived, but then goes into the finally block and kicks off another read (which again will be ignored). This seems wrong.

Packet Handling

It looks like you're using the first byte received as a packet type indicator. Depending upon the number of different packet types, you might want to break out the extraction. I would also consider breaking out the logic for constructing the packet content from the connection class. This will separate the responsibility for managing the buffer from the responsibility of managing the connection. This should make both classes easier to follow and test.