Skip to main content
added 9 characters in body
Source Link
200_success
  • 144.2k
  • 22
  • 188
  • 473

Your indentation is fine and consistent. I However, 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!)

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!)

Your indentation is fine and consistent. However, 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!)

Source Link
200_success
  • 144.2k
  • 22
  • 188
  • 473

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.