POP3 message downloader using state machine processor

This simple program uses a socket class I wrote, class socket, to retrieve a users pop3 emails and print to stdout. I would like feedback on the socket class and the code to download the pop3 emails. My main focus is on the simple state machine to go through the steps required to download the messages. It looks a bit ugly. There are probably better ways to do it.

main.cpp:

```
#include
#include
#include

#include "socket.hpp"

bool message_ok(int result, const char* buffer) {
return result != -1 && result > 3 && strncmp(buffer, "+OK", 3) == 0;
}

int main(int argc, char* argv[]) {

for (int i = 1; i
std::string user("USER ");
user += argv[3];
user += "\r\n";
ret = sock.send(user.c_str(), user.length());
ret = sock.recv(buffer, MAX_BUFFER);

if (message_ok(ret, buffer)) {
// send PASS
std::string pw("PASS ");
pw += argv[4];
pw += "\r\n";
ret = sock.send(pw.c_str(), pw.length());
ret = sock.recv(buffer, MAX_BUFFER);
std::for_each(buffer, buffer + ret, [](const char ch) { std::cout 4) {
std::string msg(buffer + 4);
size_t start = msg.find_first_of("0123456789");
size_t end = msg.find_first_not_of("0123456789");

if (start != std::string::npos && end != std::string::npos) {
std::string number(buffer + 4 + start, buffer + 4 + end);
int num = atoi(number.c_str());

for (int i = 1; i <= num; ++i) {
std::stringstream strm;
strm << "RETR " << i << "\r\n";
ret = sock.send(strm.str().c_str(), strm.str().length());

// assume that if bytes receieved == max buffer, then more to come
// problem

I would organize this code somewhat differently. But, I'm going to start with some rather minor nits, and get to the more serious "stuff" later.

I'd start by adding an overload to your socket::send that takes an std::string as its argument so you can clean up the rest of your code a little bit:

int socket::send(std::string const &s) { 
    return send(s.str(), s.length());
}

This way when you need to send a string, it'll be just:

sock.send(s);

...instead of the current code like:

sock.send(s.str(), s.length());

Likewise, I'd add an overload of recv that takes an array by reference, so the compiler can determine the array's size without your having to pass it explicitly:

template 
int recv(char (&array)[N]) { 
    return recv(array, N);
}

So when you need to receive some data, it'll just look like:

ret = sock.recv(buffer);

Then I'd rewrite msg_ok a bit. Right now it has:

return result != -1 && result > 3 && strncmp(buffer, "+OK", 3) == 0;

This appears to me to be logically redundant: a result that's greater than 3 can't possibly be equal to -1, so we might as well just eliminate the first test:

return result > 3 && strncmp(buffer, "+OK", 3) == 0;

Then we get to the real meat of things: reorganizing the code. I'd start this by creating a vector of commands that need to be sent to the server (since they're all basically similar--strings to send to the server). Assuming we can use C++14, that can look something like this:

std::vector commands { 
    {"USER "s + argv[3] + "\r\n"}, 
    {"PASS "s + argv[4] + "\r\n"},
    {"LIST\r\n")
};

[Without C++14, we need to change strings like "USER "s to std::string("USER ").]

Then I'd write a small engine to step through that vector, send each command to the server, and check the result:

for (auto const &c : commands) {
    sock.send(c);
    ret = sock.recv(buffer);
    if (!msg_ok(ret, buffer))
        throw std::runtime_error("Error retrieving mail");
    std::copy_n(buffer, ret, std::ostream_iterator(std::cout));
}

This separates two things that really are fundamentally separate: the sequence of commands we need to send to the server to retrieve mail, and the sequence of operations we need to carry out on the client to send each command, and then check and display the results.

Another possibility that might be worth considering would be to have a vector of functions that built and processed commands on the fly. This would have the advantage of making it relatively easy to process the RETR commands in roughly the same way as the others.

As far as your socket class goes, I've commented elsewhere on a roughly similar design. I think those comments apply about equally here.

StackExchange Code Review Q#106556, answer score: 3