Combine similar logic in functions GameServer::FindGameInfoClient/Server

[Brett208]

Only one body in the lines of each of these functions is partially different. The logic could probably be combined using functional programming techniques.

[DanRStevens]

I was thinking, the return type could be changed so it's a pointer to a GameSession struct, rather than an index to one.

For the error return, it can return nullptr.

---

I recommend using an error return code for this method. The method basically does packet validation. Since network packets are untrusted, we should expect validation failures as a normal occurrence. Exceptions should generally not be used for normal flow control, as exceptional flow control is much less transparent.

Actually, there have been cases of security vulnerabilities from the exception mechanism escaping past security validation code, and then using the unchecked data. I don't believe that would happen here. Just a curious note.

There are also cases of denial of service attacks, which can be amplified by slow error handling code. Exceptions are a bit slower than normal flow control, so that applies minimally here. I don't believe the difference with how we are doing things would ever be enough to matter though. Just a curious note.

---

Now that we are using C++17, we might want to consider using the [[nodiscard]] attribute. That may help ensure the error return code is checked (or at least the return value is used).

---

With a GameSession* return type, the function would then be more similar to std::find_if, making the replacement with standard library algorithms easier.

---

Related, is the FreeGameSession method, which is one of the few consumers of the index values returned by the above functions. Checking the implementation, we see FreeGameSession uses erase, which takes an interator/pointer value. This code also works natively with iterator/pointer values, rather than indexes.

The FreeGameSession method contains an error check, however this error check is for a logic error, which implies a bug in the code, and something that should be fixed, rather than a runtime error condition. We could potentially remove this error check. That reduces the method to a single line of code.

Since all that remains is a single line of code, and if the above methods return iterator/pointer values, that line of code simplifies, we could potentially remove the FreeGameSession method, in favour of directly calling standard library methods.

void GameServer::FreeGameSession(std::size_t index)
{
    // Make sure it's a valid index
    if (index >= gameSessions.size())
    {
        LogMessage("Internal Error: Tried to free a non-existent GameSession record");
        return;
    }

    gameSessions.erase(gameSessions.begin() + index);
}

Replaced by inline:

gameSessions.erase(gameSession);

[Brett208]

I am good with trying out the nodiscard attribute. I agree on removing the FreeGameSession function if the pointer syntax works out. I hadn't thought about using a pointer, which sounds like a possibly better issue than the index return. Maybe a reference would work (or maybe we could turn the pointer into a reference for calling conventions after return?). I had briefly considered removing FreeGameSession in an earlier refactor, but it got away from me.

All of this is only tangentially related to the original issue, which is combing the logic in the FindGameInfoXXX functions.

[DanRStevens]

If we use nullptr as a sentinel value for errors, then we need to use pointer syntax. References are non-nullable. It should be easy enough to convert to reference syntax in the caller after checking for nullptr.

auto resultPtr = FunctionCall();
if (resultPtr == nullptr) {
  // Error
}
auto& resultRef = *resultPtr;

And yes, a bit tangentially related to the original issue. I included it here since it may be a bit awkward to change the two areas independently.

[Brett208]

I was looking at this issue a little closer. I do not see a function to erase a member of a vector by referencing a pointer to one of its elements. I might be missing something though?

I do not believe this works:

GameSession* gameSession = [...];
gameSessions.erase(gameSession);

[DanRStevens]

Hmm, you're right. I tried changing from:

gameSessions.erase(gameSessions.begin() + index);

To:

gameSessions.erase(&gameSessions[index]);

It produces an error.

The difference is the first one uses an iterator, while the second uses a raw pointer. The erase method takes an iterator.

Close, but not quite the same thing.

[Brett208]

I'd move to just close this issue. Combining the logic flow requires using functional programming which was deemed too slow and removing vector members without referencing an index is not built into c++ standard library. There are other tasks that are probably worth pursuing at this point.

[DanRStevens]

Well, we could modify the plan. Rather than change the functions to return a pointer, we could instead modify them to return an iterator. Instead of nullptr to indicate error, we could instead return the end() iterator.

Come to think of it, std::find_if actually returns an iterator, not a pointer.

The same basic plan should work with that minor adjustment. Effectively those methods could all be replaced by calls to standard library methods.