Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve network code readability #3239

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ChillerDragon
Copy link
Contributor

The code base already started a convention of prefixing methods
that send a network message with Send

$ grep -Er 'void Send[A-Z][a-z]+\('
src/game/client/gameclient.h:   void SendKill();
src/game/server/gamecontext.h:  void SendChat(int ChatterClientID, int Mode, int To, const char *pText);
src/game/server/gamecontext.h:  void SendBroadcast(const char *pText, int ClientID);
src/game/server/gamecontext.h:  void SendEmoticon(int ClientID, int Emoticon);
src/game/server/gamecontext.h:  void SendMotd(int ClientID);
src/game/server/gamecontext.h:  void SendSettings(int ClientID);
src/mastersrv/mastersrv.cpp:void SendOk(NETADDR *pAddr, TOKEN Token)
src/mastersrv/mastersrv.cpp:void SendError(NETADDR *pAddr, TOKEN Token)
src/mastersrv/mastersrv.cpp:void SendCheck(NETADDR *pAddr, TOKEN Token)
src/versionsrv/versionsrv.cpp:static void SendVersion(NETADDR *pAddr, TOKEN ResponseToken)
src/versionsrv/versionsrv.cpp:static void SendMapversions(NETADDR *pAddr, TOKEN ResponseToken)
src/engine/client/client.h:     void SendInfo();
src/engine/client/client.h:     void SendReady();
src/engine/client/client.h:     void SendInput();
src/engine/shared/network.h:    void SendPacket(const NETADDR *pAddr, CNetPacketConstruct *pPacket);
src/engine/shared/network.h:    void SendControl(int ControlMsg, const void *pExtra, int ExtraSize);
src/engine/server/server.h:     void SendMap(int ClientID);

But not all methods followed this pattern. This commit cleans that up.

While reading the code it is now more obvious when a network message is
sent. See the following example.

Before it was hard to understand if ForceVote changes some internal
state or sends a network message.

Server()->SetRconCID(ClientID);
Console()->ExecuteLine(aCmd);
Server()->SetRconCID(IServer::RCON_CID_SERV);
ForceVote(VOTE_START_SPEC, aDesc, pReason);

Now it is clear that SendForceVote does send a message.

Server()->SetRconCID(ClientID);
Console()->ExecuteLine(aCmd);
Server()->SetRconCID(IServer::RCON_CID_SERV);
SendForceVote(VOTE_START_SPEC, aDesc, pReason);

Also chat.cpp had the methods Say and AddLine one sends a network
message the other just locally adds a new line to the chat log.

I renamed Say to SendChat which is also now matching the naming
of the server side. Any other method in chat.cpp can now be assumed to
not send any network message.

The method UpdateGameInfo(int ClientID) does not have any side
effects. It does not update any game info variables. The only thing it
does is sending the current game info to the ClientID. So the new name
SendGameInfo(int ClientID) is more descriptive.

The client sending the startinfo is a crucial part of building up
a healthy connection.

The server responds with 3 or more (depending on the amount of votes) messages.
That used to be one big wall of code that required comments to
understand where a new message starts. Now the protocol is easier to
understand while looking at the code. Without the need of comments.

if (MsgID == NETMSGTYPE_CL_STARTINFO)
{

	// [..]

	SendVoteClearOptions(ClientID);
	SendVoteOptions(ClientID);
	SendTuningParams(ClientID);
	SendReadyToEnter(pPlayer);
}

The code base already started a convention of prefixing methods
that send a network message with `Send`
But not all methods followed this pattern. This commit cleans that up.

While reading the code it is now more obvious when a network message is
sent. See the following example.

Before it was hard to understand if ForceVote changes some internal
state or sends a network message.

```C++
Server()->SetRconCID(ClientID);
Console()->ExecuteLine(aCmd);
Server()->SetRconCID(IServer::RCON_CID_SERV);
ForceVote(VOTE_START_SPEC, aDesc, pReason);
```

Now it is clear that `SendForceVote` does send a message.

```C++
Server()->SetRconCID(ClientID);
Console()->ExecuteLine(aCmd);
Server()->SetRconCID(IServer::RCON_CID_SERV);
SendForceVote(VOTE_START_SPEC, aDesc, pReason);
```

Also chat.cpp had the methods `Say` and `AddLine` one sends a network
message the other just locally adds a new line to the chat log.

I renamed `Say` to `SendChat` which is also now matching the naming
of the server side. Any other method in chat.cpp can now be assumed to
not send any network message.

The method `UpdateGameInfo(int ClientID)` does not have any side
effects. It does not update any game info variables. The only thing it
does is sending the current game info to the ClientID. So the new name
`SendGameInfo(int ClientID)` is more descriptive.
The client sending the startinfo is a crucial part of building up
a healthy connection.

The server responds with 3 or more (depending on the amount of votes) messages.
That used to be one big wall of code that required comments to
understand where a new message starts. Now the protocol is easier to
understand while looking at the code. Without the need of comments.

```C++
if (MsgID == NETMSGTYPE_CL_STARTINFO)
{

	// [..]

	SendVoteClearOptions(ClientID);
	SendVoteOptions(ClientID);
	SendTuningParams(ClientID);
	SendReadyToEnter(pPlayer);
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant