Network message handler refactoring

[karashiiro]

The network message handler in Dalamud is pretty long, and the logic is a bit convoluted. I think that extracting each of the handler branches to separate functions and then pulling them out of a couple of dictionaries keyed on opcodes will at least make it easier to read. I don't know how to make it perfectly clean (it is network code), but this might be a start. I think that ideally, each handler branch would be in a separate class, but too much state would need to be maintained between the main handler and the branch classes for that to be clean.

I'd open a PR, but this is a ~~pretty~~ very subjective readability change so I'd like to open it up for opinions.

Dalamud code: https://github.com/goatcorp/Dalamud/blob/a44e7210bc65d707da15de52303bbad62d064d35/Dalamud/Game/Network/Internal/NetworkHandlers.cs#L50-L239

Universalis ACT plugin code as an example of how this might look: https://github.com/goaaats/universalis_act_plugin/blob/ca9925f34ce25784972dd00f492f786d4c9360ef/UniversalisCommon/PacketProcessor.cs#L28-L54

[daemitus]

theres already one bit that does it correctly. just need to make the others look like this.

if (opCode == dataManager.ServerOpCodes["CfNotifyPop"])
{
this.HandleCfPop(dataPtr);
return;
} 

On Sun, Feb 6, 2022 at 2:50 PM karashiiro @.***> wrote:

The network message handler in Dalamud is pretty long, and the logic is a bit convoluted. I think that extracting each of the handler branches to separate functions and then pulling them out of a couple of dictionaries keyed on opcodes will at least make it easier to read. I don't know how to make it perfectly clean (it is network code), but this might be a start. I think that ideally, each handler branch would be in a separate class, but too much state would need to be maintained between the main handler and the branch classes for that to be clean.

I'd open a PR, but this is a pretty subjective readability change so I'd like to open it up for opinions.

Dalamud code:

https://github.com/goatcorp/Dalamud/blob/a44e7210bc65d707da15de52303bbad62d064d35/Dalamud/Game/Network/Internal/NetworkHandlers.cs#L50-L239

Universalis ACT plugin code as an example of how this might look:

https://github.com/goaaats/universalis_act_plugin/blob/ca9925f34ce25784972dd00f492f786d4c9360ef/UniversalisCommon/PacketProcessor.cs#L28-L54

— Reply to this email directly, view it on GitHub https://github.com/goatcorp/Dalamud/issues/757, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFMTBB7WI4QAM3CVJQ5CN3UZ3GI7ANCNFSM5NVXTPYA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

[karashiiro]

Yeah, that was my initial refactor in our ACT plugin, and then I realized I could stick each function into a dictionary and invoke them without explicitly branching. Here, we'd need three dictionaries though, since we have a few permutations of branch options (four if ZoneUp had any non-Universalis handlers).