Usually I'll have around 200 packets, but for the purpose of this question I've only include 1 packet in the code, the rest is the same principle as if there was still 200.
Here is a packet system, when new messages come in the ExecutePacket method will be called from the Player's class, where they'll expecute the incoming packet.
Here is the packet controller, this class handles and stores all the incoming packets.
internal class PacketController
{
private readonly Dictionary<int, IIncomingPacketEvent> _packets;
public void ExecutePacket(Player player, IncomingPacket packet)
{
if (!_packets.TryGetValue(packet.Header, out var packetEvent))
{
Logger.Log("Unhandled packet: " + packet.Id);
return;
}
Logger.Log("Handled packet: " + packet.Id);
packetEvent.Execute();
}
public void LoadPackets()
{
LoadHandshake();
}
private void LoadHandshake()
{
_packets.Add(IncomingPacketHeaders.GetClientKeyEvent, new GetClientKeyEvent());
}
}
Incoming and Outgoing packets have headers, which in simple terms are identifiers (IDs).
class IncomingPacketHeaders
{
public const int GetClientKeyEvent = 2042;
}
class OutgoingPacketHeaders
{
public const int SendSpecialKey = 9254;
}
The rest of the code
interface IIncomingPacketEvent
{
void Execute();
}
class GetClientKeyEvent : IIncomingPacketEvent
{
public void Execute()
{
// Execute the packet...
}
}
Almost forgot, OutgoingPackets are "composers" (what we call them).
internal class SendSpecialKey : OutgoingPacket
{
public SendSpecialKey() : base(OutgoingPacketHeaders.SendSpecialKey)
{
WriteString("some special key");
}
}
Here are the IncomingPacket and `OutgoingPacket classes also.
public class IncomingPacket
{
private byte[] _body;
private int _pointer;
public int Header;
public IncomingPacket(int messageId, byte[] body)
{
Load(messageId, body);
}
public int RemainingLength => _body.Length - _pointer;
public void Load(int messageId, byte[] body)
{
if (body == null)
{
body = new byte[0];
}
Header = messageId;
_body = body;
_pointer = 0;
}
public void AdvancePointer(int i)
{
_pointer += i * 4;
}
public byte[] ReadBytes(int bytes)
{
if (bytes > RemainingLength)
{
bytes = RemainingLength;
}
var data = new byte[bytes];
for (var i = 0; i < bytes; i++)
{
data[i] = _body[_pointer++];
}
return data;
}
public byte[] PlainReadBytes(int bytes)
{
if (bytes > RemainingLength)
{
bytes = RemainingLength;
}
var data = new byte[bytes];
for (int x = 0, y = _pointer; x < bytes; x++, y++)
{
data[x] = _body[y];
}
return data;
}
public byte[] ReadFixedValue()
{
return ReadBytes(EncodingUtilities.DecodeInt16(ReadBytes(2)));
}
public string ReadString()
{
return PlusEnvironment.GetDefaultEncoding().GetString(ReadFixedValue());
}
public bool ReadBoolean()
{
return RemainingLength > 0 && _body[_pointer++] == Convert.ToChar(1);
}
public int ReadInt()
{
if (RemainingLength < 1)
{
return 0;
}
var data = PlainReadBytes(4);
var i = EncodingUtilities.DecodeInt32(data);
_pointer += 4;
return i;
}
}
And outgoing
public class OutgoingPacket
{
private readonly List<byte> _body;
private readonly Encoding _encoding;
protected readonly int Id;
public OutgoingPacket(int id)
{
_body = new List<byte>();
_encoding = Encoding.Default;
Id = id;
WriteShort(id);
}
public void WriteByte(int b)
{
_body.Add((byte) b);
}
private void WriteBytes(IReadOnlyList<byte> b, bool isInt) // d
{
if (isInt)
{
for (var i = b.Count - 1; i > -1; i--)
{
_body.Add(b[i]);
}
}
else
{
_body.AddRange(b);
}
}
public void WriteDouble(double d)
{
var raw = Math.Round(d, 1).ToString(CultureInfo.InvariantCulture);
if (raw.Length == 1)
{
raw += ".0";
}
WriteString(raw.Replace(',', '.'));
}
public void WriteString(string s)
{
WriteShort(s.Length);
WriteBytes(_encoding.GetBytes(s), false);
}
public void WriteShort(int s)
{
WriteBytes(BitConverter.GetBytes((short) s), true);
}
public void WriteInteger(int i)
{
WriteBytes(BitConverter.GetBytes(i), true);
}
public void WriteBoolean(bool b)
{
WriteBytes(new[] {(byte) (b ? 1 : 0)}, false);
}
public byte[] GetBytes()
{
var final = new List<byte>();
final.AddRange(BitConverter.GetBytes(_body.Count));
final.Reverse();
final.AddRange(_body);
return final.ToArray();
}
}
I won’t share the client side code, as that's totally unrelated to the question.
1 Answer 1
There are a lot of small things to say, I've certainly missed something, but here goes!
Endianness
It is apparent that you have thought about endianness from the isInt parameter which is good, and while this will certainly work fine on most machines, you shouldn't be assuming the endianness of the system: check IsLittleEndian to determine that, and only reverse the bytes of supplied by BitConverter.GetBytes(*) if it is not the correct endianness (I assume you are using Big ('Network') Endian).
Readonly Fields
You have suitably marked the fields in OutgoingPacket as readonly: the same treatment should be given to those in IncomingPacket, esspecially Header which is currently publically settable.
Incoming/Outgoing Encoding Mismatch
Consider IncomingPacket.ReadString(): I do not like that this jumps into some other part of the code to read strings which were written out by a simple short-prefixed string in an encoding that is accessed by a different mechanism. This looks like a maintainability nightmare, because changes to the PlusEnvironment.GetDefaultEncoding() could easily be made without updating OutgoingPacket. These coupled methods should be defined together, so that it is obvious they are coupled.
The same issue applies almost everywhere. My suggestion would be to extend EncodingUtilities to provide GetBytes style methods so that all of the concerns with endianness and encoding are handled in one place, and tear out all of the 'custom' stuff in OutgoingPacket
Making up data
In IncomingPacket.ReadInt() you have
if (RemainingLength < 1)
{
return 0;
}
What is this doing?! There is nothing in OutgoingPacket that enables you to crop an int if it happens to be zero and the last value: what utility can this check possibly provide?
If you are trying to read an int, and there is no int to read, you should probably be throwing an exception. Missing data isn't something the system should be dealing with by making it up: the programmer has made an error and they want to know about it, so tell them in the most helpful way possible by throwing an exception.
The same comments can be made for IncomingPacket.(Plain)ReadBytes(int bytes).
if (bytes > RemainingLength)
{
bytes = RemainingLength;
}
I asked for bytes bytes, don't give me fewer than bytes bytes! (Incidentally, I don't like the variable name bytes, it sounds like a buffer: count or byteCount would seem clearer). Again: throw an exception: tell me there were not enough bytes when you first realise.
PacketController.ExecutePacket(Player, IncomingPacket)
This code never passes the packet to the packet event: you have all this code to read/write the payload for the packet, but you don't seem to be doing anything with it.
// Execute the packet...
Which packet?!
IncomingPacket.ReadBoolean()
... I feel I must be missing something: why are you comparing a byte to a char?
return RemainingLength > 0 && _body[_pointer++] == Convert.ToChar(1);
Why not just compare the byte to the byte value 1?
Again, RemainingLength > 0 is helping no-one.
I'd be inclined to pull out a method that reads a single byte (e.g. ReadByte()), because currently this one line of code (I'd expand the if as well) does a number of jobs.
/// <Summary> Reads a single byte from the stream </Summary>
public byte ReadByte()
{
if (RemainingLength < 1)
throw new IncomingPacketException("Attempted to read a Byte from a consumed packet");
return _body[_pointer++];
}
/// <Summary> Reads a boolean value from the stream </Summary>
public bool ReadBoolean()
{
return ReadByte() == (byte)1;
}
You have a public OutgoingPacket.WriteByte(byte) method, so it seems silly not to have a public IncomingPacket.ReadByte() method also.
IncomingPacket.ReadInt()
This is the only method that uses the PlainReadBytes(int) method. Why does it not just use ReadBytes(int)? Why is PlainReadBytes(int) even provided?
You are loading tasks onto ReadInt() (_pointer += 4) that should be done by ReadBytes(int), which is inconsistent with most other methods in OutgoingPacket.
OutgoingPacket.WriteByte(int b)
Why is this taking an int?! WriteByte(??) should take a _byte_! If, as a consumer of this method, I want to write an int and pretend that it is a byte, then I should have to perform that cast myself. All you've done here is written in an unclear API which will help to introduce copy-and-paste errors. In the very least, this behaviour of casting should be documented.
OutgoingPacket.WriteBytes(IReadOnlyList<byte>, bool)
I'd be inclined to break this into two methods, one for things where endianness matters, and one for things where endianness doesn't matter. This makes it much clearer at the call-site that the correct method is being used, rather than anyone looking at the code having to understand the second parameter and remember which way round it is.
OutgoingPacket.WriteDouble(double)
You are already using CultureInvarient, there is no need to replace commas with points.
Is there a good reason you are manually appending a ".0" to a double which encodes to a single digit?
All this double stuff is a bit scary, because it looks like a general purpose writer, but in reality it is cropping your double, and this is completely undocumented. If your entire system is built around the concept of not having more than one decimal of precision, then that's OK (it probably still warrants a comment) but if not, then this looks like a great way to confuse someone unfamiliar with the code.
All that said, there is no IncomingPacket.ReadDouble() method, so this method is kind of useless.
Outgoing.WriteBool(bool b)
I'd have this lean on WriteByte(byte), it's just cleaner.
WriteByte((byte)(b ? 1 : 0));
OutgoingBytes.GetBytes()
I don't like that this uses a different set of code for writing an endian-flipped integer... but you'd need a heavy refactoring to change this.