NetPlayServer.cpp OnConnect() Minor Code Updates

[Sage-King]

Removed code duplicating comments, renamed a few variables, and updated some syntax.

No functional changes AFAIK

[AdmiralCurtiss]

Please clean up the commit history, there's no reason to have changes that you revert within the same PR.

[phire]

Not sure I agree with your definition of "code duplicating comment"

I agree that many of them aren't great comments, but I do feel like the code is slightly more readable with the comments.

[Sage-King]

Not sure I agree with your definition of "code duplicating comment"

I agree that many of them aren't great comments, but I do feel like the code is slightly more readable with the comments.

Looking back at this, yes, some of them are a little more readable with comments. Some of them are definitely code duplication though. (looking specifically at the npver->netplay_version name change and related comment) Is there a way we can remove these comments? Maybe put those more readable comments as function names and the relevant functionality within those functions? I doubt it'd change anything as far as the assembly is concerned, but it'd make it a lot more readable. If we could put those as __force_line__ in the header, we could know for sure they're not changing anything.

[phire]

The netplay version, and the "game running or pending" are the two least useful comments there.

If we could put those as force_line in the header

You are overthinking things; If it improves readability, just split it out. This is not hot code, it runs once per player.

Though I find splitting things out into small, single-use functions can actually hinders readability, as you now have to jump to the function to check what it does, an inline comment can be much more readable.

While "self-documenting code" is good, I don't think you should ever rewrite something in a more self-documenting way just to avoid writing a comment (or worse, delete an existing comment).

If I was cleaning that function up, I'd be focusing on removing the verbose send_packet.clear() and stream based packet construction. Try and refactor it so packets can be initialized, constructed and sent with just a single function call. With the goal being to make the whole function shorter and less verbose.

And then instead of deleting most of those comments, rewrite then to explain "why"

[phire]

I'm just going though and commenting explictly on all the comments I think we shouldn't outright delete

[Sage-King]

@phire I didn't follow your directions exactly, but I think I've found a happy medium between function wrapping and commenting in this version.

[phire]

Yeah, they were suggestions, not directions.