SFML-based chat in C++

Question

I wanted to get some review on this code I wrote. The repo is here if you want to download the CodeBlocks project.

This program is several things: it is a console using SFML graphics; it is a console able to parse a few hardcoded commands (this is how clients and servers will start the networking); and it is a server/client model chat program.

To start your server, you type "listen". To connect to a server at IP 100.100.100.100, you type "connect 100.100.100.100". To set your name to Bob, you type "setname Bob".

I have noted some minor flaws in the finished product, but I want some other eyes on my code.

Main.cpp:

#include <SFML/Graphics.hpp>
#include "Console.h"
#include <thread>
int main()
{
 // Create the main window
 sf::RenderWindow window(sf::VideoMode(640, 480), "SFML window");
 //window.setKeyRepeatEnabled(false);
 Console console;
 bool windowOpen = true;
 std::thread networkThread([&] {console.updateSockets(windowOpen);} );
 networkThread.detach();
 // Start the game loop
 while (window.isOpen())
 {
 // Process events
 sf::Event event;
 while (window.pollEvent(event))
 {
 // Close window : exit
 if (event.type == sf::Event::Closed)
 {
 console.disconnect();
 windowOpen = false;
 window.close();
 }
 console.update(event);
 };
 // Clear screen
 window.clear();
 console.showUI(window);
 console.showChat(window);
 // Update the window
 window.display();
 }
 return EXIT_SUCCESS;
}

Console.h:

#ifndef CONSOLE_H
#define CONSOLE_H
#include <SFML/Graphics.hpp>
#include <SFML/Network.hpp>
#include <string>
#include <list>
#include <mutex>
struct IpPair
{
 sf::IpAddress ipaddress;
 unsigned short port;
};
class Console
{
 public:
 Console();
 virtual ~Console();
 void update(const sf::Event& event);
 void getInput(const sf::Event& event);
 void sendMessage();
 void sendSystemMessage(std::string text);
 bool parseCommand();
 void show(sf::RenderWindow& window);
 void showChat(sf::RenderWindow& window);
 void showUI(sf::RenderWindow& window);
 sf::Text makeText(const std::string str);
 void updateSockets(const bool& windowOpen);
 void attemptClientConnect();
 void clientSend();
 void clientDisconnectSend();
 void clientReceive();
 void attemptServerStart();
 void serverReceive();
 void serverSend();
 void serverDisconnectSend();
 void disconnect();
 protected:
 private:
 sf::Font font;
 std::vector<sf::Text> chat;
 std::string userText;
 std::string userName;
 enum Role {Disconnected, Client, Server} role;
 enum MessageType {Normal, Disconnecting} messageType;
 sf::UdpSocket socket;
 IpPair server;
 bool clientReady, serverReady;
 std::vector<std::string> messagesToSend;
 std::vector<IpPair> clients;
 std::mutex messagesM, chatM;
};
#endif // CONSOLE_H

Console.cpp:

#include "Console.h"
#include <SFML/Graphics.hpp>
#include <SFML/Network.hpp>
#include <string>
#include <vector>
#include <iostream>
Console::Console()
{
 userName = "No Name";
 font.loadFromFile("opensans.ttf");
 socket.setBlocking(false);
 server.port = 12000;
 clientReady = false;
 serverReady = false;
 role = Role::Disconnected;
}
Console::~Console()
{
 //dtor
}
//Main update function
void Console::update(const sf::Event& event)
{
 getInput(event);
}
//Functions used by or related to the update function below here
void Console::getInput(const sf::Event& event)
{
 if (event.type == sf::Event::KeyPressed)
 {
 if (event.key.code == sf::Keyboard::Return)
 {
 if (userText.size() > 0)
 sendMessage();
 }
 else if (event.key.code == sf::Keyboard::BackSpace)
 {
 if (userText.size() > 0)
 userText.pop_back();
 }
 else if (event.key.code == sf::Keyboard::Space)
 {
 userText += ' ';
 }
 }
 else if (event.type == sf::Event::TextEntered)
 {
 if (32 < event.text.unicode && event.text.unicode < 128)
 userText += (char)event.text.unicode;
 }
}
void Console::sendMessage()
{
 if (parseCommand())
 return;
 std::string finalString = userName + ": " + userText;
 sf::Packet packet;
 userText.clear();
 switch (role)
 {
 case Role::Disconnected:
 sendSystemMessage(finalString);
 break;
 default:
 messagesM.lock();
 messagesToSend.push_back(finalString);
 messagesM.unlock();
 break;
 }
}
void Console::sendSystemMessage(std::string text)
{
 chatM.lock();
 chat.push_back(makeText(text));
 chatM.unlock();
}
bool Console::parseCommand()
{
 std::string::iterator itor;
 //Replace with generic parsing function:
 //std::string parseUntil(std::string fullCommand, char stop, std::string::iterator &itor);
 //From:
 std::string command;
 for (itor = userText.begin(); *itor != ' ' && itor != userText.end(); ++itor)
 {
 command += *itor;
 }
 ++itor;
 //To here. Returns command, itor is moved along since it's a reference.
 std::transform(command.begin(), command.end(), command.begin(), ::tolower);
 if (command == "connect")
 {
 std::string fullIP = userText.substr(std::distance(userText.begin(), itor));
 server.ipaddress = fullIP;
 sendSystemMessage((std::string)"Attempting to connect to " + server.ipaddress.toString() + ".");
 userText.clear();
 role = Role::Client;
 return true;
 }
 else if (command == "listen")
 {
 sendSystemMessage("Beginning server.");
 userText.clear();
 role = Role::Server;
 return true;
 }
 //Would like name saved locally.
 else if (command == "setname")
 {
 userName = userText.substr(std::distance(userText.begin(), itor));
 sendSystemMessage((std::string)"Name changed to \'" + userName + "\'.");
 userText.clear();
 return true;
 }
 return false;
}
//Main show (draw) function
void Console::show(sf::RenderWindow& window)
{
 showUI(window);
 showChat(window);
}
//Functions called by or related to the show function below this line
void Console::showChat(sf::RenderWindow& window)
{
 chatM.lock();
 if (chat.size() > 0)
 {
 std::vector<sf::Text>::reverse_iterator rev;
 int count = 0;
 for (rev = chat.rbegin(); rev != chat.rend(); ++rev)
 {
 rev -> setPosition(20, 412 - count*34);
 ++count;
 window.draw(*rev);
 }
 }
 chatM.unlock();
}
void Console::showUI(sf::RenderWindow& window)
{
 std::string stringVersion = userName + ": " + userText;
 sf::Text text = makeText(stringVersion);
 text.setPosition(20, 446);
 window.draw(text);
}
sf::Text Console::makeText(const std::string str)
{
 sf::Text text;
 text.setFont(font);
 text.setCharacterSize(14);
 text.setFillColor(sf::Color::White);
 text.setString(str);
 return text;
}
//Main networking function
void Console::updateSockets(const bool& windowOpen)
{
 while (windowOpen)
 {
 switch (role)
 {
 default:
 break;
 case Role::Client:
 {
 attemptClientConnect();
 clientSend();
 clientReceive();
 break;
 }
 case Role::Server:
 {
 attemptServerStart();
 serverReceive();
 serverSend();
 break;
 }
 }
 }
}
//Functions called by or related to the networking function below this line
void Console::attemptClientConnect()
{
 if (!clientReady)
 {
 if (socket.bind(sf::UdpSocket::AnyPort) == sf::UdpSocket::Done)
 {
 clientReady = true;
 char port[50];
 sprintf(port, "%d", socket.getLocalPort());
 std::string firstMessage = userName + " is connecting...";
 messagesM.lock();
 messagesToSend.push_back(firstMessage);
 messagesM.unlock();
 sendSystemMessage((std::string)"Success. Socket bound on " + port + ". Say hello!");
 }
 else
 {
 role = Role::Disconnected;
 sendSystemMessage("Binding the socket has failed.");
 }
 }
}
void Console::clientSend()
{
 messagesM.lock();
 for (int i = 0; i < messagesToSend.size(); ++i)
 {
 sf::Packet packet;
 packet << MessageType::Normal << messagesToSend.at(i);
 for (int i = 0; i < 3; ++i)
 {
 if (socket.send(packet, server.ipaddress, server.port) != sf::UdpSocket::Done)
 {
 std::cout << "Send failed. Try " << i << " out of 3." << std::endl
 << "Server IP: " << server.ipaddress.toString() << std::endl;
 }
 else break;
 }
 }
 messagesToSend.clear();
 messagesM.unlock();
}
void Console::clientDisconnectSend()
{
 sf::Packet packet;
 packet << MessageType::Disconnecting << userName + " has disconnected.";
 messagesM.lock();
 for (int i = 0; i < 3; ++i)
 {
 if (socket.send(packet, server.ipaddress, server.port) != sf::UdpSocket::Done)
 {
 std::cout << "Send failed. Try " << i << " out of 3." << std::endl
 << "Server IP: " << server.ipaddress.toString() << std::endl;
 }
 else break;
 }
 messagesM.unlock();
}
void Console::clientReceive()
{
 sf::Packet packet;
 IpPair ip;
 int mt;
 if (socket.receive(packet, ip.ipaddress, ip.port) == sf::UdpSocket::Done)
 {
 packet >> mt;
 if (mt == MessageType::Disconnecting)
 {
 role = Role::Disconnected;
 }
 std::string finalString;
 if (packet >> finalString)
 sendSystemMessage(finalString);
 else
 sendSystemMessage((std::string)"Packet read failed: " + ip.ipaddress.toString());
 }
}
void Console::attemptServerStart()
{
 if (!serverReady)
 {
 if (socket.bind(server.port) == sf::UdpSocket::Done)
 {
 char port[50];
 sprintf(port, "%d", socket.getLocalPort());
 sendSystemMessage((std::string)"Success! Listening on port " + port);
 serverReady = true;
 userText.clear();
 }
 else
 {
 sendSystemMessage("Binding the socket has failed.");
 role = Role::Disconnected;
 userText.clear();
 }
 }
}
void Console::serverReceive()
{
 IpPair ip;
 sf::Packet packet;
 if (socket.receive(packet, ip.ipaddress, ip.port) == sf::UdpSocket::Done)
 {
 bool found = false;
 int mt;
 packet >> mt;
 for (int i = 0; i < clients.size(); ++i)
 {
 if (clients.at(i).ipaddress == ip.ipaddress
 && clients.at(i).port == ip.port)
 {
 if (mt == MessageType::Disconnecting)
 {
 clients.erase(clients.begin() + i);
 }
 found = true;
 break;
 }
 }
 if(!found)
 {
 sendSystemMessage((std::string)"Incoming connection: " + ip.ipaddress.toString());
 clients.push_back(ip);
 sendSystemMessage("Added " + ip.ipaddress.toString());
 }
 messagesM.lock();
 std::string msg;
 if(packet >> msg)
 {
 messagesToSend.push_back(msg);
 }
 messagesM.unlock();
 }
}
void Console::serverSend()
{
 messagesM.lock();
 for (int i = 0; i < messagesToSend.size(); ++i)
 {
 sendSystemMessage(messagesToSend.at(i));
 for (int j = 0; j < clients.size(); ++j)
 {
 bool good = true;
 sf::Packet sendPacket;
 sendPacket << MessageType::Normal << messagesToSend.at(i);
 for (int i = 0; i < 3; ++i)
 {
 if (socket.send(sendPacket, clients.at(j).ipaddress, clients.at(j).port) != sf::UdpSocket::Done)
 {
 std::cout << "Send failed for " << clients.at(j).ipaddress << ":" << clients.at(j).port << ". Try " << i << " out of 3." << std::endl;
 if (i == 2)
 {
 good = false;
 }
 }
 else break;
 }
 if (!good)
 {
 messagesM.lock();
 messagesToSend.push_back((std::string)"A client has been disconnected.");
 messagesM.unlock();
 clients.erase(clients.begin() + j);
 }
 }
 }
 messagesToSend.clear();
 messagesM.unlock();
}
void Console::serverDisconnectSend()
{
 sf::Packet packet;
 packet << MessageType::Disconnecting << (std::string)"The server has shut down.";
 messagesM.lock();
 for (int j = 0; j < clients.size(); ++j)
 {
 for (int i = 0; i < 3; ++i)
 {
 if (socket.send(packet, clients.at(j).ipaddress, clients.at(j).port) != sf::UdpSocket::Done)
 {
 std::cout << "Send failed for " << clients.at(j).ipaddress << ":" << clients.at(j).port << ". Try " << i << " out of 3." << std::endl;
 }
 else break;
 }
 }
 messagesM.unlock();
}
void Console::disconnect()
{
 switch (role)
 {
 case Role::Client:
 clientDisconnectSend();
 break;
 case Role::Server:
 serverDisconnectSend();
 break;
 default:
 break;
 }
}

Answer 1

I just have some small notes, as your program is overall very good:

1. Why virtual destructor? You never actually inherit from Console. If you want to be on the safe side if someone inherits from it eventually, mark Console as final to prevent it being used as base class. Also, your destructor does nothing, and as such the default destructor should be used.

2. You never actually use/call Console::show. You should, and at the same time, mark showUI and showChat as private. That's because those 2 functions should never be called without the other one, that's also why you have a show function, to guarantee that both are called.

3. Don't use C-style casts:

   userText += (char)event.text.unicode;
   

   Instead, use static_cast (in your case), or some other C++ cast.

   Casting a string literal to std::string is also dangerous, it is better if you use std::string literals:

   using namespace std::string_literals;
   sendSystemMessage("Attempting to connect to "s + server.ipaddress.toString() + "."s);
   

4. Don't use the default reference capture in a lambda, for the same reason as mutable globals: You might modify a variable you were not supposed to, among other reasons.

5. I wouldn't actually create an object for the thread you create, networkThread, as you immediately detach it. Creating a temporary would work as well:

   std::thread{ [&console, &windowOpen] {
    console.updateSockets(windowOpen);
   } }.detach();
   

6. You don't need to return EXIT_SUCCESS;, the compiler does it for you.

7. Why do you have an empty protected in Console? You could remove it.

8. Use an enum class instead of an enum, as for the later the enum values exist in the scope where the enum was created, possibly conflicting with declarations with the same name. You also have implicit conversions to int, which can lead to weird usage of them.

9. You don't use IpPair anywhere except in Console, so you might as define it in class scope.

10. Use the member initialization list to initialize variables in the constructor which depend on passed parameters, and initialize them inline for default values, instead of its body.

11. Along with point 2, make getInput a private function, as update already calls it. Alternately, rename getInput to update and remove the old update altogether.

    Also make any internal helper functions private, as well as any function that shouldn't be called by someone outside of the class.

12. Use a std::scoped_lock (or std::lock_guard if you can't use C++17) to lock and free mutexes. That's because of an exception is thrown between lock and unlock, the mutex is never released.

13. Instead of your loop in parseCommand and iterators, you can just something like:

    auto command = userText.substr(0, userText.find(' '));
    auto parameters = userText.find(' '); // instead of 'itor'
    if (parameters != std::string::npos)
     ++parameters;
    

    This also avoid the undefined behavior in your code, when you increment itor possibly past the end iterator.

14. You don't use rev in showChat outside of the for loop, so why declare it outside? If you were worried about the line length, use auto:

    for (auto rev = chat.rbegin(); rev != chat.rend(); ++rev)
    {
     // ...
    }
    

15. Use std::to_string instead of std::sprintf.

16. I also like specifying any function that never throws as noexcept, as that can help the compiler when it optimizes. Except if you are compiling without exceptions of course.

17. Use the at function on containers if you are not sure if the index is in range. In an index loop over the container, the index is always in range (if you don't modify the container), so you can use operator[] instead, to remove the unnecessary overhead of the range check.

18. I would make a separate class for the client, the server, the backend and the console, to not have a "god" class which does everything.

I think that's it :) Keep it up.

• jacobdavis1: You are amazing! Some of these, like the virtual destructor, are just because CodeBlocks made them that way, or I saw it in an example, and I just didn't know any better. Some of them were because it was the first way of doing something I discovered. I will do my best to implement all of these! Edit: I forgot to mention: you are absolutely right about the show function; I guess it slipped my mind to update main. And again, you are right that I should make all of those other functions private in Console; that's the whole point of centralizing the functions, haha.