Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

How can I improve the string RecvData() function?

std::string Socket::RecvData() {
    std::string strBuffer;
    do{
        char buffer;
        int recvInt = recv(s_, &buffer, 1, 0);
        if (recvInt == INVALID_SOCKET)
        {
            return "";
        }
        if (recvInt == SOCKET_ERROR)
        {
            if (errno == EAGAIN) {
                return strBuffer;
            }
            else {
                // not connected anymore
                return "";
            }
        }

        strBuffer += buffer;
        if (buffer == '\n')  return strBuffer;
    } while (true);
}
share|improve this question

4 Answers 4

up vote 2 down vote accepted

I'll purely suggest some changes to code structure, but as @user58697 states, you should carefully check the man page to recv and handle each and every error condition.

The first thing to do is to break up the code a bit. Secondly, a lot of people prefer a single return point. The record terminator should be a defined constant.

Capture the concepts of a valid read and the desire to discard buffered data in methods.

Probably matching the record terminator should also really be broken out into a method.

Also recv returns ssize_t.

class Socket
{
public:
    std::string RecvData();
    static const char recordTerminator = '\n';
private:
    int s_;
    bool validRead(ssize_t recvCode);
    bool discardBuffer(ssize_t recvCode);

};
bool Socket::validRead(ssize_t recvCode)
{
    return (recvCode != INVALID_SOCKET && recvCode != SOCKET_ERROR);
}
bool Socket::discardBuffer(ssize_t recvCode)
{
    return (recvCode == INVALID_SOCKET || 
        (recvCode == SOCKET_ERROR && errno != EAGAIN));
}
std::string Socket::RecvData() 
{
    std::string stringRead;
    bool readMore = true;
    while(readMore)
    {
        char singleChar = 0;
        ssize_t recvInt = recv(s_, &singleChar, 1, 0);
        readMore = validRead(recvInt);
        if (readMore)
            stringRead += singleChar;
        else if (discardBuffer(recvInt))
            stringRead = "";
        if (readMore && (singleChar == recordTerminator))
            readMore = false;       
    } 
    return stringRead;
}
share|improve this answer
1  
By getting rid of the early returns, you've made the looping behaviour harder to understand. Early returns are generally a good thing, except that it may make Return Value Optimization trickier. –  200_success Apr 15 at 4:17
1  
@200_success and in C++11 moving basically negates the problem of a potential return copy, though of course you can incur the cost of a move instead of the relatively zero cost of RVO. –  Corbin Apr 15 at 7:07
1  
Secondly, a lot of people prefer a single return point. I think that i very old school C. C++ is designed so that multiple return points no longer causes an issue. See RAII. –  Loki Astari Apr 15 at 14:48
    
I don't like the validRead() or discardBuffer() they don;t add anything to the code and make following the logic harder. –  Loki Astari Apr 15 at 14:50

recv() is a system call, which has relatively high overhead. You should avoid calling recv() just to read one byte at a time. Instead, read a reasonable sized buffer (something like the size of an IP packet). Your wrapper object should maintain a buffer of bytes that have been returned by a previous call to recv() but that have not yet been returned to the caller (bytes after a newline character).

In implementing such a buffered interface, you would be forced to use the return value of recv() correctly. recv() returns the number of bytes read, or -1 if an error occurred. The error code is stored in the errno global variable, not the return value of recv().

share|improve this answer

I was asked if I can explain my recommendations in the code. Here it goes. Take it with a grain of salt.

int Socket::RecvLine(std::string strBuffer)
{
    while (1) {
        char ch;
        switch(rc = recv(s_, &buffer, 1, 0)) {
        case 0:     // End-of-stream, the peer closed connection
            return DONE;
        case 1:     // Got something
            strBuffer += ch;
            if (ch == '\n')
                return OK;
            break;
        case -1:    // Some kind of error; the code below is just for reference.
                    // Don't use it in production.
            if (errno == EINTR || errno == EAGAIN)
                continue;
            return ERROR;
        }
    }
}

As 200_success mentioned, recv is expensive, and you really want to read as much as availabe, which means a secondary buffering is in order of the day. That's next step.

share|improve this answer
  1. Change the name. The function returns when it sees a newline, so the correct name is RecvLine or something similar.

  2. recv returns -1 on any error. The way your code is structured, it would bail out without testing the errno.

  3. On EAGAIN you should not return, but continue. Also you may want to do something more intelligent on other errors, such as EINTR.

  4. You must account for the recvInt to become 0, which actually means "not connected anymore".

In any case you should not abandon the data already collected, so return strBuffer anyway, along with some indication of the reason to return. This suggests a different interface to your method, such as int RecvLine(std::string &strBuffer).

share|improve this answer
1  
can you exaplane in code ? –  user3238714 Apr 15 at 3:06
2  
Agree with everything apart from the interface. An interface where you have to check for error codes is error prone (which is the problem with a lot of C interfaces). You should have an interface that is hard to misuse by the user. –  Loki Astari Apr 15 at 3:30
    
Let's agree to disagree. The callee doesn't have a knowledge of how/why it was called, and if it may terminate for various reason its only option is to report the reason for termination (in this case, reasons are EVERYTHING_FINE, DONE and UNRECOVERABLE_ERROR). Nobody but the caller may decide on what to do next. PS: I am more than happy to discuss problems with C interfaces, which are plenty. –  vnp Apr 15 at 5:38
    
Returning error codes is great internally. It allows you to fix bugs there and then and you have to make sure you fix all error codes and account for them. But leaking them across your public API is a bad idea. The problem is that users of your code often forget to check for error states. A better alternative is to throw an exception. This will force application termination if there is an error that is not explicitly handled (which is what you want because if the error is not handled the application is doing something unintended and therefore probably breaking some assumptions. –  Loki Astari Apr 15 at 15:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.