Reading a line of text from a socket

Question

I tend to write code, with very few comments, and few line breaks. I'm currently just a student at a University. I've had professors tell me one way or another to comment, but I have a hard time with more ambiguous tasks.

I have a misc. snippet of code, written by me in C++. I'm not looking for revisions on it or anything. Just how, in your professional field, would you go about commenting it?

CString Socket::recvln()
{
    int size, count;
    char* result;

    OSFactory::lockMutex(recvLock);
    size = recvBuffer.size();
    result = new char[size+1];
    for (count = 0; count < size; count++)
        if (recvBuffer.at(count) == '\n')
            break;
        else
            result[count] = recvBuffer.at(count);

    if (count >= size) // Should never >= size if '\n' found
        count = 0;
    result[count] = '\0';
    recvBuffer.erase(recvBuffer.begin(), recvBuffer.begin()+count);
    OSFactory::unlockMutex(recvLock);

    std::string _result = result;
    delete[] result;
    return _result;
}

Again, this is just a misc. method I have. I'm just trying to understand how other people would go about commenting code. There are tons of tutorials out there, but many even have you comment getter/setter methods, and to me that's really really pointless. Many times it's very clear that setName(std::string), for example, sets the Name!

So, I'm just trying to have a better grasp on real life commenting, versus text-book answers. For some reason this is really hard for me, and I'm sure it should benefit other people as well.

Answer 1

I don't see anything in this code that needs to be commented. To echo what other people said, comments should clarify code and they should never say what the code already says.

I would recommend commenting code under these conditions:

1. You can't tell what the code is doing by reading it
2. You encountered a wonky bug that took you a long time to figure out, and the fix does not look clear or uses a programming construct or pattern that isn't normally used.

Really, the biggest problem I see is one of the biggest problems in Software Engineering. As Martin Fowler once said:

... there are two hard things in computer science: cache invalidation, naming things, and off-by-one errors.

The names of your variables and methods could use a little polishing.

For instance, does Socket::recvln() mean:

1. Receive Line
2. Receive Lane
3. Recover Line...

Even if names become a little long, make them obvious. Don't abbreviate. Some people say that abbreviating well known terms within your industry are OK. I'm from the camp that disagrees. Never assume that the next programmer is an expert in your industry.

When it comes to "reducing lines" I have mixed feelings about omitting optional curly brackets. It looks nicer, but becomes extra typing when you have to add one line of code. I go back and forth on that and have come to a compromise with myself.

• If omitting the curly brackets makes the code a two-liner, that's OK

  if (count >= size)
      count = 0;
  

• If I would be omitting curly brackets for nested structures, I'll put the curly brackets in even if I don't need to

  for (count = 0; count < size; count++)
  {
      if (recvBuffer.at(count) == '\n')
      {
          break;
      }
      else
      {
          result[count] = recvBuffer.at(count);
      }
  }
  

• But sometimes within a nested structure, I'll omit the optional brackets if it is obvious:

  for (count = 0; count < size; count++)
  {
      if (recvBuffer.at(count) == '\n')
          break;
      else
          result[count] = recvBuffer.at(count);
  }
  

• If I have a structure with one-liners and multi liners, I never omit the curly brackets:

  if (count > size)
  {
      count = 0;
  }
  else
  {
      foo = count + 10;
      bar = foo * -1;
  }
  

Lastly, I recommend putting spaces around operators to make the code easier to read:

recvBuffer.erase(recvBuffer.begin(), recvBuffer.begin() + count);

(notice the spaces around the "+" sign)

Answer 2

Commenting the method

At the minimum, public API methods should always be commented.

The posted code is an implementation. You should have the method declaration, serving as the interface definition, somewhere else. It's best when the actual implementation is not even visible to users of your API. That's good information hiding.

So, where this method is declared, you should add comments explaining what it does and what it returns.

It could also use a better name, as recvln is overly and pointlessly succinct instead of receive_line.

Commenting inside the method

Probably like you, I also don't like commenting. I like when the code is readable by itself. When I feel the need to add comments, that's a sign that perhaps there are better ways to write the code.

Looking at this code, although some of it is straightforward and easy to understand, there are elements that are not clear at all. Let's refactor it and see where that leads us.

---

Why are these declarations at the top?

int size, count;
char* result;

This was mandatory in C, but not in C++. It's not ideal to declare things up front, it's much more readable when variables are declared closer to where they are used, and ideally initialized at the same time.

By moving the declarations further down and combining with the initialization, the code becomes shorter, and now more information becomes apparent and easily visible, the purpose of these variables:

int size = recvBuffer.size();
char* result = new char[size+1];

---

In this code:

for (count = 0; count < size; count++)
    if (recvBuffer.at(count) == '\n')
        break;
    else
        result[count] = recvBuffer.at(count);

Why does recvBuffer.at(count) appear twice? In fact, it seems in most iterations it will be evaluated twice, which is pointless. So let's improve that:

for (count = 0; count < size; count++) {
    char c = recvBuffer.at(count);
    if (c == '\n') break;
    result[count] = c;
}

I also removed the else line: it's just not needed due to the break as the result of the if.

Notice how the code is getting shorter and shorter.

---

What is this about?

if (count >= size) // Should never >= size if '\n' found
    count = 0;

Logically, count will be <= size always. And that comment is not accurate enough, which makes it confusing instead of helping.

It would be better like this:

// if '\n' was not found, reset count
if (count == size) count = 0;

Actually, this comment could be avoided by using a boolean variable:

bool foundNewLine = false;
for (count = 0; count < size; count++) {
    char c = recvBuffer.at(count);
    if (c == '\n') {
        foundNewLine = true;
        break;
    }
    result[count] = c;
}

if (!foundNewLine) count = 0;

Now a comment would be pointless, the code speaks for itself.

---

Putting it together the method becomes:

OSFactory::lockMutex(recvLock);

int size = recvBuffer.size();
char* result = new char[size+1];

int count;
bool foundNewLine = false;
for (count = 0; count < size; count++) {
    char c = recvBuffer.at(count);
    if (c == '\n') {
        foundNewLine = true;
        break;
    }
    result[count] = c;
}

if (!foundNewLine) count = 0;

result[count] = '\0';
recvBuffer.erase(recvBuffer.begin(), recvBuffer.begin() + count);

OSFactory::unlockMutex(recvLock);

std::string _result = result;
delete[] result;
return _result;

This code is as clean as I can make it, and now it seems clear enough that I don't feel the need to add comments.

About vertical spacing, I added blank lines between blocks that are not tightly related.

Conclusion

Maybe only poorly written code needs comments. If you feel the need to add comments, in other words you find the code is not readable by itself, then it's poorly written. And if it's poorly written, then it's not comments it needs, but refactoring. So, keep refactoring until it doesn't need comments.

This is a great article about coding without comments.

Answer 3

Usually when I comment I look over the code and see what might be unclear to other people reading the code, and then I try to explain that line in plain english. What's clear to you about your own code isn't always as clear to others. So a good tip would also be to have others look over your code, ask them what they don't understand about the code and then write comments based on their feedback.

This for example might be good to write a comment on:

recvBuffer.erase(recvBuffer.begin(), recvBuffer.begin()+count);

Even if it's just a brief explanation of what it does. It will also help you in the future if you ever go back to check out your own code. You might be thinking to yourself "what the hell does this even do...?", then you will appreciate even commenting on the smallest things. As a personal opinion I think it's better to comment too much than too little.

Answer 4

Formatting

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

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.

Answer 5

I find by using the standard libraries as much as possible you can reduce the number of comments you need because they are well understood.

I don't know the actual types of some of the variables used in your code so this example may very well not work in your case. But similar code might be rewritten a bit like this:

CString Socket::recvln()
{
    std::string result;

    OSFactory::lockMutex(recvLock);

    auto found = std::find(recvBuffer.begin(), recvBuffer.end(), '\n');

    result.assign(recvBuffer.begin(), found);

    if(found != recvBuffer.end())
        ++found; // skip '\n'

    recvBuffer.erase(recvBuffer.begin(), found);

    OSFactory::unlockMutex(recvLock);

    return result;
}

I could only find room for one comment, being the reminder that the terminating character needs to be skipped. This might help with future maintenance (those gosh darn off-by-one errors!).

Answer 6

If you want to do a good job at commenting you need to write down the preconditions, post conditions and non-obvious gotchas (which should be avoided). Something like this:

    CString Socket::recvln();
    /*
    Reads a line from the socket and returns it in a CString.
    A line ends with a '\n'-character.
    The '\n'-character is consumed but not included in the return value.
    The behavior is undefined unless the Socket object is open and can be read from.
    If EOF is encountered before reading a '\n'-character the read data is returned.
    If a stream error is encountered an instance of Stream::Stream_error is thrown
    and the read data is discarded.
    */

This is an example, your actual code does things differently, but you should see where this is going.

The function never returns a string containing a newline. I am unsure if this invariant should be documented, but you could write a unit test for it.

The documentation should go into the declaration, not the definition.

Adding comments for the implementation is difficult, because I only see the obvious things that are not worth pointing out whereas I cannot see and comment the hidden gotchas that are worth commenting. One thing that jumps at me is that you manually lock and unlock a mutex. If a socket error occurs you leak the lock, creating deadlocks later. Either document that //WARNING: Function may leak a lock and cause deadlocks!!! or, if I missed your safety mechanisms, state that: //This function can never leak OSFactory::lockMutex. You can do that using RAII classes such as std::lock_guard. Same thing for manually using new and delete, std::make_unique would fix that and make the code easier.
I would also want a comment explaining why you internally create an std::string which then gets converted to CString. Bug or feature?
It is also worth mentioning that recvBuffer is used internally to buffer data. All other reading functions must check that first before doing a read operation on the socket, which becomes extremely important when adding new reading functions later. (You remember now, but not next week).

See Defensive Programming Done Right part (1) and (2) for more information.