Winsock recv all bytes

Question

I had some issues with sending huge data and receiving them with Winsock so I made some improvements on my RecvAllBytes function and the same with SendAllBytes. I would like you to review it because I made a lot of conditions and loops. Please let me know how to improve the code below.

constexpr int maxBufferSize = 8192;
bool Server::RecvAllBytes(std::shared_ptr<Connection> connection, char * data, int totalbytes)
{
 int totalbytesreceived = 0;
 if (totalbytes > maxBufferSize)// if buffer is bigger then maximum allowed 
 {
 while (totalbytesreceived < totalbytes)//while full buffer not received 
 {
 int bytesrecv = 0;
 int bytesleft = totalbytes - totalbytesreceived;
 if (bytesleft >= maxBufferSize)// if there is still more left then maximum size keep recv full size
 {
 while (bytesrecv < maxBufferSize)
 {
 int ReturnCheck = recv(connection->socket, data + totalbytesreceived, maxBufferSize - bytesrecv, NULL);
 if (ReturnCheck == SOCKET_ERROR)
 {
 return false;
 }
 bytesrecv += ReturnCheck;
 totalbytesreceived += ReturnCheck;
 }
 }
 else// if left less then maximum size recv last bytes left 
 {
 while (bytesrecv < bytesleft)
 {
 int ReturnCheck = recv(connection->socket, data + totalbytesreceived, bytesleft - bytesrecv, NULL);
 if (ReturnCheck == SOCKET_ERROR)
 {
 return false;
 }
 bytesrecv += ReturnCheck;
 totalbytesreceived += ReturnCheck;
 }
 }
 }
 }
 else // if buffer is not bigger then maximum allowed
 {
 while (totalbytesreceived < totalbytes) {
 int ReturnCheck = recv(connection->socket, data + totalbytesreceived, totalbytes - totalbytesreceived, NULL);
 if (ReturnCheck == SOCKET_ERROR) {
 return false;
 }
 totalbytesreceived += ReturnCheck;
 }
 }
 return true;
}

Answer 1

Regarding improving the code, here are some suggestions:

1. Reduce the number of conditions/loops. The whole method is doable in a single while loop. See below for an example.
2. Prefer nouns for names of variables and verbs for functions/methods. For instance, instead of calling the return value ReturnCheck, call it ReturnValue.
3. Refactor the code to handle the connection getting closed before getting all of the expected bytes. According to a Microsoft Winsock web page for the recv call, "If the connection has been gracefully closed, the return value is zero". At which point I believe your code will try again and get SOCKET_ERROR (with an error code of WSAENOTCONN). And then return false even though it may have missed only say the last byte expected.
4. Recognize values that are constant with the C++ const type qualifier (or constexpr as you've done for compile-time constants).
5. Seeing as how you used constexpr (available since C++11), go ahead and also prefer using auto.
6. Consider making the RecvAllBytes method a method of a class that the client can use too (I don't demonstrate this though).

Here's how this could look (I didn't compile and test this code but hopefully it works):

int Server::RecvAllBytes(std::shared_ptr<Connection> connection, char * data, int bytesExpected)
{
 WSASetLastError(0);
 auto bytesReceived = decltype(bytesExpected){0};
 while (bytesExpected > 0)
 {
 const auto bytesRequested = (bytesExpected > maxBufferSize)? maxBufferSize: bytesExpected;
 const auto returnValue = recv(connection->socket, data + bytesReceived, bytesRequested);
 if (returnValue == -1 || returnValue == 0)
 {
 return bytesReceived;
 }
 bytesReceived += returnValue;
 bytesExpected -= returnValue;
 }
 return bytesReceived;
}

And how it could be used:

const auto bytesReceived = server.RecvAllBytes(connection, bigBuffer, bigSize);
if (bytesReceived < bigSize)
{
 if (WSAGetLastError() != 0)
 {
 // handle error.
 }
 else
 {
 // handle connection closed before all bytes received.
 }
}