Network programming for an IRC Bot

Question

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 <iostream>
#include <string>
#include <vector>
#include <sstream>
#include <regex>
#include <algorithm>

#include <SFML/Network.hpp>

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

enum class ServerMessage {Ping, ConnectionEstablished};

std::vector<std::string> split(const std::string& str, const std::string& delimiter) {
    std::vector<std::string> 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 << "Sending data: " + data << std::endl;
    if (sck->send(data.c_str(), data.length()) != sf::Socket::Done)
    {
        std::cout << "Error on sending " << data << std::endl;
        return false;
    }
    return true;
}

ServerMessage handle(const std::string& msg, sf::TcpSocket* sck)
{
    std::cout << msg << std::endl;
    if (msg.substr(0, 4) == "PING")
    {
        auto payload = msg.substr(6, msg.length() - 6);
        std::cout << payload << std::endl;
        send("PONG :" + payload + "\r\n", sck);
        return ServerMessage::Ping;
    }
    else
    {

        std::regex connectedMessage{ "^:.* 001 NimBot" };
        if(std::regex_search(msg, connectedMessage)) {
            return ServerMessage::ConnectionEstablished;
        }
    }

}

void receive(sf::TcpSocket* sck)
{
    char rcvData[255];
    memset(rcvData, 0, ARRAY_LEN(rcvData));
    std::size_t received;
    if (sck->receive(rcvData, ARRAY_LEN(rcvData), received) != sf::Socket::Done)
    {
        std::cout << "oops" << std::endl;
    }

    std::string rcvString(rcvData);

    auto parts = split(rcvData, "\r\n");
    std::for_each(parts.begin(), parts.end(), [sck](std::string& s) { handle(s, sck); });
}

void waitForConnect(sf::TcpSocket* sck)
{
    bool connected = false;

    char rcvData[255];
    while (!connected) {

        memset(rcvData, 0, ARRAY_LEN(rcvData));
        std::size_t received;
        if (sck->receive(rcvData, ARRAY_LEN(rcvData), received) != sf::Socket::Done)
        {
            std::cout << "oops" << std::endl;
        }

        std::string rcvString{ rcvData };

        auto parts = split(rcvData, "\r\n");
        std::for_each(
            parts.begin(),
            parts.end(),
            [sck, &connected](std::string& s) { if(handle(s, sck) == ServerMessage::ConnectionEstablished) connected = true; });
    }
    std::cout << "CONNECTED!" << std::endl;
}

bool establishConnection(sf::TcpSocket* sck)
{
    sf::Socket::Status status = sck->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 (!establishConnection(&sck))
    {
        return 1;
    }

    if (!loginOnIRC(&sck))
    {
        return 1;
    }
    waitForConnect(&sck);
    send("JOIN " + channel + "\r\n", &sck);
    send("PRIVMSG " + channel + " :Hello!\r\n", &sck);
    while (true)
    {
        receive(&sck);
    }
    return 0;
}

Answer 1

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<std::string> 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<char, 255> 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 <typename Func>
void do_receive(sf::TcpSock* sck, Func f)
{
    std::array<char, 255> 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.

Answer 2

Just a little comment. I see this pattern used way too often in C++ code:

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

That's fine and dandy if you are writing C, but macros are a very primitive tool to be using on a language that has metaprogramming capabilities (AKA templates). The correct way to implement such construct in C++ is using a template function:

template<class T, std::size_t N>
constexpr std::size_t arrayLength(const T (&)[N])
{
    return N;
}

Thanks to the constexpr, it can be used in any context the macro would, plus, you get all the goodies of a function (i.e.: scoping, clearer error messages, type-safety, etc).

---

That being said, most of time you should be using std::array anyways, but for those rare cases where a plain array is still needed, this function is the best solution, IMHO.

Answer 3

I just want add to what Yuushi wrote, that you don't seem to use any classes.

I'd encapsulate the networking within it's own class. Other than that, Yuushi was pretty thorough.

If you'd like an example (but with Boost.Asio), you can look at my network code my IRC bot in development at GitHub.