Network programming for an IRC Bot

I'm currently making my first steps into the territory of network programming, using the SFML Network library to simplify things.

Later on, my bot shall post new pull requests and changes concerning issues to an IRC channel.

I've got the bot to connect to a server (irc.euirc.net is hardcoded for now) and to a channel, leaving a "Hello!" message.

I'm currently unhappy with the realization of the receive and waitForConnect functions, which are basically doing the same. Could someone give me a hint how to combine both methods into one while retaining the current functionality?

```
#include
#include
#include
#include
#include
#include

#include

#define ARRAY_LEN(x) (sizeof(x)/sizeof(*x))

enum class ServerMessage {Ping, ConnectionEstablished};

std::vector split(const std::string& str, const std::string& delimiter) {
std::vector splitted;

std::string s{ str };
size_t pos{ 0 };
std::string token;
while ((pos = s.find(delimiter)) != std::string::npos) {
token = s.substr(0, pos);
splitted.push_back(token);
s.erase(0, pos + delimiter.length());
}

return splitted;
}

bool send(const std::string& data, sf::TcpSocket* sck)
{
std::cout send(data.c_str(), data.length()) != sf::Socket::Done)
{
std::cout receive(rcvData, ARRAY_LEN(rcvData), received) != sf::Socket::Done)
{
std::cout receive(rcvData, ARRAY_LEN(rcvData), received) != sf::Socket::Done)
{
std::cout connect("irc.euirc.net", 6667, sf::seconds(5.0f));
if (status != sf::Socket::Done)
{
std::cout << "Error on connect!" << std::endl;
return false;
}
std::cout << "Connect was successful!" << std::endl;
return true;
}

bool loginOnIRC(sf::TcpSocket* sck)
{
send("NICK NimBot\r\n", sck);
send("USER NimBot :Nimelrians Bot\r\n", sck);
return true;
}

int main()
{
const std::string channel{ "#nimbottest" };
sf::TcpSocket sck{};
if (!establishConnecti

Here are some of the notes i jotted down while reading through your code:

If you're going to make a copy of a string that is passed in by const reference, you might as well pass it by value and then move it. For example, in split:

std::vector split(std::string str, const std::string& delimiter)
{
    // Just use str directly without having to copy it, as it already is
    // a copy.
    ...
}

Similarly with substr. This returns a new string, so you might as well just use this directly (and get the benefits of having an rvalue that gets moved, instead of copied):

splitted.push_back(s.substr(0, pos));

Instead of having errors print out on std::cout, you might want to have them go to std::cerr instead. This makes it much easier to isolate just the error messages (for example, by redirecting it via piping using 2> errors.txt).

Since your handle function is going to be called relatively often, you might want to consider making the regex inside it static. Further, since it isn't modified, it should be const static. This will be both threadsafe and will save you having to recreate the same regex every time the function is called.

const static std::regex connectedMessage{"^:.* 001 NimBot" };

Instead of using raw arrays here, using std::array will probably make your life a bit easier. It'll get rid of having to use macros to calculate array sizes, and having to use memset.

void receive(sf::TcpSocket* sck)
{
    std::array rcvData;
    // Each element is default initialized, no need to call memset.
    std::size_t received;
    if (sck->receive(rcvData.data(), rcvData.size(), received) != sf::Socket::Done) {
        ...
    }
}

Often use of algorithms rather than raw for loops is better, but I think use of std::for_each should probably be deprecated, and use of range-based for loops should take priority since C++11. For example in receive, I think:

for(const auto& part : parts) {
     handle(part, sck);
}

is more readable compared to the for_each with a lambda.

Overall, these are pretty minor nitpicks.

In terms of reducing the duplication in receive and waitForConnect, the biggest difference is the function that gets called in your for_each. The easiest way would be to create another function that takes a templated parameter:

template 
void do_receive(sf::TcpSock* sck, Func f)
{
    std::array data;
    std::size_t received{0};
    if (sck->receive(rcvData, ARRAY_LEN(rcvData), received) != sf::Socket::Done)
    {
        std::cerr << "Socket recieve did not return Done!" << std::endl;
    }

    std::string rcvString(rcvData);
    auto parts = split(rcvData, "\r\n");
    for(const auto& part : parts) {
        f(part, sck);
    }
}

This could then be called by both receive and waitForConnect:

void receive(sf::TcpSocket* sck)
{
    auto func = [](const std::string& part, sf::TcpSocket* sck)
                { handle(part, sck); };
    do_receive(sck, func);
}

And similarly for waitForConnect using a slightly different lambda.

As a suggestion for future improvements, you might want to eventually do other things with the program instead of just the network component. Since this is single-threaded at the moment, and just sits there blocking on the call to receive, you might want to look at launching it in a separate thread.

StackExchange Code Review Q#93509, answer score: 11