Repository navigation
Messages (Handler/Sender): Refactor Packet Handling and Sending into a DDD approach. #182
Description
Right now all the Messages handled by a Channel, both to Handle packets coming from the Client-Side and also to Send Packets to the Server-Side are being handled by two files:
Network/ChannelPacketHandlerNetwork/ChannelPacketSender
Those two files are already enormously big and already might impact the maintainability of the Melia Core.
The suggestion is to first move the part of the code where messages are being handled and written to a different part of the Emulator. (It is important to mention to leave all-kind of Business-Logic and Application-Logic outside the scope of composing the bodies and reading the bodies of packets.
This means that the refactored methods for handling and sending packets (The part that composes the bodies of the messages and the part that reads them) should be isolated.
Sender-Reader-Handler Approach
- A reader should only contain pieces of code to read the Packet and return the data that was read as a return value of the method
- A Sender should only contain pieces of code to write the bodies of the packets and return the Buffer ready to be sent.
- A handler should invoke a Reader function when it wants to read a packet and Invoke a Sender function when it wants to send something.
An example of this strategy would be:
private readonly CharacterSessionReader Reader = new CharacterSessionReader();
private readonly CharacterSessionSender Sender = new CharacterSessionSender();
[PacketHandler(Op.CZ_LOGOUT)]
public void CZ_LOGOUT(ChannelConnection conn, Packet packet)
{
var { extra, unkByte } = Reader.CZ_LOGOUT();
Log.Info("User '{0}' is logging out.", conn.Account.Name);
Send(Sender.ZC_SAVE_INFO(conn));
Send(Sender.ZC_LOGOUT_OK(conn));
}Domain-Driven-Design Folder Structure (Code Separation)
Regarding the DDD part of the refactoring, it would mean that the Readers and Senders and Handlers would be related to their Functionality based in the Game, rather than what they are (Senders/Readers).
Imagine the following folder structure:
- Communication/Messages
- Character/
- Session/
- Reader.cs
- Handler.cs
- Sender.cs
- Inventory
- Reader.cs
- Handler.cs
- Sender.cs
...
...
This is an example of a possible Folder structure. The naming varies based on how we would separate and group the Messages.
An example of this strategy would be:
- Messages related to the Character should reside in the bigger Group "Character"
- Messages specific to the Character to manage its Inventory should reside inside "Character/Inventory"...
- Messages related to Managing the state/effects of Monsters should reside in the bigger Group "Monsters"
- Messages related to the Character to use Skills should reside inside "Character/Skills"
This is just an illustrative example.
Activity
exectails commentedon Sep 4, 2021
Shouldn't this be a discussion for now? In any case, I've been going back and forth on this for 10 years with other developers, and I still always come back to the current design for its simplicity.
First, there are the readers and writers/senders. I agree that, generally, it would make sense to put those into separate classes. It's clean, you only ever see that one packet, you can unit test them, but from a server emulator coder's perspective, it's a really annoying design. Every single time you want to add a packet handler, you have to go create one or two new files for the reader and the writer and implement them. Then you go and create the actual handler and potentially sender. Taking hopping between files into account, you spend what feels like 10x the amount of time on just adding the basic handling for a packet, especially because you often times don't get a packet right on the first try. And this adds up quickly, especially in these early stages, when you're constantly adding new packets. And then, whenever you need to make changes, you're hopping back and forth between files again.
Adding categorization on top of of that makes it even worse, because then you're always asking yourself where you should put something, where you put things in the past, where things should that go which's purpose you don't know yet, if maybe you should be moving something, etc.
In my eyes, this doesn't work well for a server emulator, where requirements constantly change and evolve and new discoveries change how packets are used and interpreted. It just adds additional steps, but offers very little benefit in my opinion. It lowers the file size of these classes a little, yes, but is that significant? When I need to find a packet I use the reference search and jump there immediately. The size of the overall file matters little, and I see the entire packet handling at one glance, which in most cases is exactly what I need in our use case. Organizing the packets "properly" also lets you unit test them, certainly a point, weren't it for the fact that packets are in constant flux and whether they're correct isn't determined by our unit tests, but the client. We aren't in control here after all.
Under normal circumstances I wouldn't use our current design. I would use something more or less like you proposed. But if I don't know what's coming and I constantly have to add, remove, modify, and tweak packets, putting everything in one place makes things much easier and straight forward in my opinion. 4000 lines of packet senders is a lot, yes. But personally, I think I'd rather split that class up into partial classes at some point, if actually necessary, instead of adding literally over 1000 separate files.