Winsock recv Function Optimization

[Zhuge]

I wrote this function for an TCP, Winsock, socket-based text game I was working on, and was wondering if anyone had any ideas for improvements.

int recvFromSocket(SOCKET From, string& Storage) {
	string temp;
	char buff[2] = {'\0', '\0'};
	int bytes = 0;
	bool done = false;
	while(!done) {
		if(From == INVALID_SOCKET) {
			closesocket(From);
			return SOCKET_ERROR;
		}
		bytes = recv(From, buff, 1, 0); //get one char from the socket
		if(bytes <= 0) { //if nothing was stored
			closesocket(From); //close the socket
			return SOCKET_ERROR; //return an error
		} else if(buff[0] == '\b' && !temp.empty()) temp.erase(temp.size() - 1); //get rid of the last stored char if it's a backspace and isn't empty
		else if(buff[0] == '\r') {
			//create timeval struct
			timeval timevalInfo; // tells select how long to wait
			timevalInfo.tv_sec = 0;
			timevalInfo.tv_usec = 10; // wait for 10
			//made fd_set struct
			fd_set Sockets; // contains list of sockets for select
			Sockets.fd_count = 1; // one socket
			Sockets.fd_array[0] = From; // add it
			//call select to determine readability of function
			int sret = select(NULL, &Sockets, NULL, NULL, &timevalInfo);
			if(sret == SOCKET_ERROR) return WSAGetLastError();
			else if(sret == 0) { //if time expired
				Storage.assign(temp); //there is no \n so continue
				done = true;
			} else { //there is more data (it is readable)
				char peek[2] = {'\0', '\0'};
				bytes = recv(From, peek, 1, MSG_PEEK); //check to see if it \n
				if(bytes <= 0) { //if nothing was stored
					closesocket(From); //close the socket
					return SOCKET_ERROR; //return an error
				}
				if(peek[0] == '\n') { //if it is
					bytes = recv(From, peek, 1, 0); //actually get the \n
					if(bytes == 0) { //if nothing was stored
						closesocket(From); //close the socket
						return SOCKET_ERROR; //return an error
					}
					Storage.assign(temp);
					done = true;
				}
			}
		} else if(buff[0] == '\n') { //if it is at an enter
			Storage.assign(temp); //assign the stuff to the storage string
			done = true; //exit ok
		} else { //if not at an end (there is more)
			temp.append(buff); //assign the stuff
			bytes = 0; //reset bytes, and loop
			done = false;
		}
	}
	return NO_ERROR;
};

[berkcan]

"Premature optimization is the root of all evil." - Knuth, Donald

You need to run a profiler on your code and idetify the bottleneck. You can just optimize without having a target or a goal. Try IBM's Rational Purify as a profiler.

I'll still you a code review though. See my comments:

- Have your function return from only one point. Currently it returns from all over the place. It makes debugging harder.
- Avoid using magic numbers. You have two char arrays of size 2. Make 2 a const variable.
- The only place where you return WSAGetLastError(), you do it before closing the socket.
- In general, you have too many nested if's and else's. This increases code complexity. Noone wants to debug or maintain complex code. I am sure there is a way to cleanup your logic in this function.

Berk Celebisoy
Software Developer - Microsoft Corp.

[Duthomhas]

It looks to me like you are trying to do too much.

If I understand it correctly, you want to receive one 'line' of text from the socket (that is, a string that is separated from other strings in the socket input stream by newlines) --something like 'getline( mysocket, mystring )'.

A few of observations:

1) If From is an INVALID_SOCKET, there is no need to close it (it isn't open to begin with). The caller shouldn't have been able to get past the bind() and listen() functions or the connect() function before calling your recvFromSocket().

2) Lines 11 and 12 make a dangerous assumption, one that may (or may not, who knows?) actually fail, since SOCKET is #defined as an unsigned int).

3) Blocking for socket input is always a very bad idea: it can lead to a locked program. I would just use select() first to see if there is any incoming data, and only then recv() it. (Even if the other end closes without first sending the explicit "I'm gonna close" signal, the select() function will still signal you to read that empty message.)

4) The return value is also relying upon the actual values of NO_ERROR (zero), SOCKET_ERROR (~zero), etc. I personally think it would be a better idea to return something like a bool and let the caller call WSAGetLastError() to see what went wrong... or something like that.

5) For speed issues, avoid creating temporaries and read as many characters as you can from the socket at a time. (And again, don't recv() unless select() says it is OK --blocking is bad.) You will need to buffer any extra characters received between calls.

Just some thoughts.

Personally, I think it is worth it to wrap everything into a little class to do everything (connect, disconnect, gather messages, etc.) for you. I wrote something similar not too long ago --perhaps I'll post back with a little C++ version for you.