Formatting
Your indentation is fine and consistent. I strongly discourage you from omitting the optional braces with for
, if
, and else
. Every time you do so, you are contributing to a future coding accident. (Don't think that it can't happen to you!)
You may wish to consider indenting the block of code in the critical section, so that the pairing of lockMutex()
and unlockMutex()
calls is obvious. If RAII-style locking is possible, that would be even better — a destructor would take care of the unlocking for you.
Commenting
The most important comment is the documentation of the method. If your method is well designed and the code is straightforward, then you won't need much additional commenting, if at all.
To that end, I think the code could benefit from some simplification, as it is a bit too convoluted to spot what is going on. Some issues:
- The declaration of
size
and count
can be postponed, and therefore probably should be.
- You
new
/delete[]
a char[]
, then make a std::string
from it, which then gets converted into a CString
. Why not just let result
be a CString
, and write directly to its buffer?
- Checking
(count >= size)
is a clumsy way to determine whether a newline was encountered.
In the code below, I have assumed that a .copy(dest, begin, end)
method exists (and if it doesn't, there probably should be something similar).
/**
* Returns received data up to, but excluding, the first '\n'.
* If no newline has been received so yet, an empty string is returned.
*/
CString Socket::recvln()
{
CString result;
OSFactory::lockMutex(recvLock);
{
int size = recvBuffer.size();
for (int count = 0; count < size; count++)
{
if (recvBuffer.at(count) == '\n')
{
LPTSTR buf = result.GetBuffer(count + 1);
recvBuffer.copy(buf, recvBuffer.begin(), recvBuffer.begin() + count);
buf[count] = '\0';
result.ReleaseBuffer();
recvBuffer.erase(recvBuffer.begin(), recvBuffer.begin() + count);
break;
}
}
}
OSFactory::unlockMutex(recvLock);
return result;
}
Bug?
I would think that it would be better to erase the '\n'
from the buffer
recvBuffer.erase(recvBuffer.begin(), recvBuffer.begin() + count + 1);
so that the next call to .recvln()
will not return a string with a leading newline.