As in #9579, indentation and further documentation can be done later to avoid further disruption on the same PR. Maybe at different times for different groups of functions. The main goal of this PR ProcessMessage to make it more readable and maintainable (ie, changes to ProcessMessage should in principle be easier to review after this PR). Brings ProcessMessage to 142 lines.
This should be relatively easy to review.
What problem is this PR fixing?
Right, readability and maintainability (ie ease of review for later changes on ProcessMessage). As noted, this can be indented later (although that obscures the git blame). In other projects, having to read more than X nested control flows in function directly goes against the style and smaller functions can be always a solution to that. Others simply don’t allow functions above N lines
IMO, before this PR ProcessMessage is both too long and has some lines with too many nestings. It is very hard for me to read this function as it, and I suspect I’m not the only one.
I feel like this just obscures the control flow. :-/
Yeah, how so?
(although that obscures the git blame).
Use git blame -w.
2657+ else if (strCommand == NetMsgType::PONG)
2658+ {
2659+ return MsgPong(pfrom, vRecv, nTimeReceived);
2660+ }
2661+ else if (strCommand == NetMsgType::FILTERLOAD)
2662+ {
How does it obscure the control flow? Because there is only one caller to these functions, and they’re all called in order. But now looking at the function to considering its invariants you can no longer see where they are called from, what threads they run, what orders things happen in… As far as nesting goes… it hardly changes the nesting– just reduces the indentation by one. It doesn’t change the cyclomatic complexity of the code (which is usually the motivation against large functions, avoiding gnarly intertwined functions that are not testable).
I don’t think it grievously hurts things, but I do not see the improvement and it feels a bit like movement for movement sake to me. No biggie.
ProcessMsg[type](). You’re proposing to refactor SendMessages() in #9579. I’d prefer that function to be split into SendMsg[type]() in that PR if you continue. At the moment you have a function here MsgFeefilter() for processing a received feefilter message and a function in #9579 called MsgFeefilter() for sending a feefilter message. That’s not helpful for code taging and documentation.
Concept ACK, but I don’t see this as an improvement without separating control flow and dispatch. I believe that’s part of @gmaxwell’s concern as well?
Added that here: https://github.com/theuni/bitcoin/commit/f1e4e281e3f1eb884f8010ac941c82752174bdbe (needs tests). This has the advantage of moving the rules for message acceptance into one place. If the message is deemed suitable, it can be dispatched with no ordering/dependency concerns.
