Clean up NetClient.cpp

[Stan]

A few function suffer from the recent refactorings in Netclient.cpp CNetClient::TryToConnect has a few redundant checks, and some missing ones.

For instance we use g_XmppClient->SendStunEndpointToHost without checking if it is nullptr.

Bonus: StunClient::FindPublicIP has some potential case of failures that are not tested.

[Stan]

In 25908:

Fix false positive; undefined variable in NetworkClient.cpp

refs #6321 for further cleanups.
Discussed with: @wraitii
Differential Revision: ​https://code.wildfiregames.com/D4258

[intensity]

Hey all, first time potential contributor, just wanted to get some general direction.

Question 1

In CNetClient::TryToConnect I took this initial attempt at fixing the logic for g_XmppClient checks. ​https://code.wildfiregames.com/differential/diff/18720/

is that what the ticket was referring to?

Question 2

What's the general desired strategy here? Take the g_XmppClient example:

 208 void CNetClient::SetupConnectionViaLobby()
 209 {
 210     g_XmppClient->SendIqGetConnectionData(m_HostJID, m_Password, m_UserName.ToUTF8(), false);
 211 }

Solution 1

if (!g_XmppClient)
{
    LOGERROR("blah bad thing");
    return;
}

but then we'd need to address what it means to have had this "not happen" in the calling functions.

Solution 2

Change return type to bool and return false and then update callers to handle that correctly.

Solution 3

Add (and/or move) a call of ENSURE(!!g_XmppClient) into SetupConnectionViaLobby such that callers themselves don't need to know that they need to check that this global is set.

[wraitii]

I would say 'yes' for Q1 but IMO the indentation level is not great and I'm not sure it's worth fixing. Perhaps worth making a sub function instead.

For your Q2, I'd encourage you to get some more 'data' on why one solution would perhaps be best, by checking the context of the calling code & other similar functions.

As a general rule:

• Stuff that can fail in the 'happy path' of the game should not warn, so then a boolean return would probably be best
• Stuff that might fail in weird cases/misconfigured configs/bad mods should possibly warn, but not crash.
• Stuff that should never happen should ENSURE. if the ENSURE is triggered, then the bad code is earlier in the call hierarchy and should be fixed there.

[Stan]

Unlikely to get done for A26

[Langbart]

Adding the patch number

[Freagarach]

Pushing back.