14
\$\begingroup\$

I'm working on a project (studying) and I have a lot of time for proof of concepts and write something from scratch.

Basically I'm creating an HTTP server (simple, but not too simple) in C++ using sockets and multi-threading.

There are two topics that I'm concerned about: the design pattern of my code structure and the efficiency of my thread implementation. Obs: I'm a little worried about C++ best practices, since I'm diving too fast into C++ (am I abusing of std items, since this requires a low-level implementation).

Server is the main file, that calls Routes, Request and Response. Request and Response contains Struct (that contains Status Code). And Routes contains Request and Response.

IMPORTANT: I'm using nlohmann/json and j-ulrich status-codes.

Server (Accepting Sockets and Multi-threading):

#pragma once

#include <unistd.h>
#include <stdio.h>
#include <sys/socket.h>
#include <stdlib.h>
#include <netinet/in.h>
#include <string.h>
#include <iostream>
#include <unordered_map>
#include <thread>
#include <mutex>
#include <condition_variable>

#include "Server/Request.h"
#include "Server/Response.h"
#include "Server/Struct.h"
#include "Server/Routes.h"
#include "Tools/Logger.h"

class Server {
    public:
        Server(unsigned int port, unsigned int max_connections = 64, unsigned int thread_count = 5);
        ~Server();

        bool setRoute(std::string path, Struct::Methods method, void (*callback)(Request*, Response*));
        bool doListen();
        bool doStop();

    private:
        unsigned int _port;
        unsigned int _max_connections;
        unsigned int _thread_count;

        std::mutex _mutex;
        std::condition_variable _condition;

        bool _signal;

        std::vector<unsigned int> _queue;

        std::thread* _thread_consume;
        std::thread* _thread_process;

        Routes* _routes;

        int _socket;
        struct sockaddr_in _address;
        bool _listen;

        bool _doStop();
        bool _doCreateSocket(int& socket_in);
        bool _doBindSocket(int file_descriptor);
        void _doConsumeSocket();
        void _doProcessSocket(int id);
        bool _doProcessRequest(Request* request, Response* response);
};

#include "Server/Server.h"

Server::Server(unsigned int port, unsigned int max_connections, unsigned int thread_count) {
    if (port > 65535) {
        Logger::doSendMessage(Logger::TYPES::ERROR, "[Port must be something between 0 and 65535 on Server::Constructor.");
    }

    if (max_connections < 1) {
        Logger::doSendMessage(Logger::TYPES::ERROR, "Max connections can't be lower than 1 on Server::Constructor.");
    }

    _port = port;
    _max_connections = max_connections;
    _thread_count = thread_count;

    _routes = new Routes();

    int status = _doCreateSocket(_socket);
    if (!status) {
        Logger::doSendMessage(Logger::TYPES::ERROR, "Failed to create socket on Server::Constructor.");
    }

    if (!_doBindSocket(_socket)) {
        Logger::doSendMessage(Logger::TYPES::ERROR, "Failed to bind socket on Server::Constructor.");
    };

    _signal = false;
    _listen = false;
}

Server::~Server() {
    _doStop();

    shutdown(_socket, SHUT_RD);
    close(_socket);

    try {
        _thread_consume->join();
        for (size_t i = 0; i < _thread_count; i++) {
            _thread_process[i].join();
        }
    } catch (...) {}

    delete _thread_consume;
    delete[] _thread_process;
    delete _routes;
}

bool Server::setRoute(std::string path, Struct::Methods method, void (*callback)(Request*, Response*)) {
    return _routes->setRoute(path, method, callback);
}

bool Server::doListen() {
    if (_listen) return false;

    int status;

    status = listen(_socket, _max_connections);
    if (status < 0) return false;

    Logger::doSendMessage(Logger::TYPES::INFO, "Server running with success at port " + std::to_string(_port) + ".");

    _listen = true;

    _thread_consume = new std::thread(&Server::_doConsumeSocket, this);
    _thread_process = new std::thread[_thread_count];
    for (size_t i = 0; i < _thread_count; i++) {
        _thread_process[i] = std::thread(&Server::_doProcessSocket, this, i);
    }

    return true;
}

bool Server::doStop() {
    return _doStop();
}

bool Server::_doStop() {
    if (!_listen) return false;
    {
        std::lock_guard<std::mutex> lock(_mutex);
        _listen = false;
    }
    _condition.notify_one();
    return true;
}

bool Server::_doCreateSocket(int& socket_in) {
    int file_descriptor = socket(AF_INET, SOCK_STREAM, 0);
    if (file_descriptor == 0) return false;

    int error;
    int opt = 1;

    error = setsockopt(file_descriptor, SOL_SOCKET, SO_REUSEADDR | SO_REUSEPORT, &opt, sizeof(opt));
    if (error) return false;

    socket_in = file_descriptor;
    return true;
}

bool Server::_doBindSocket(int file_descriptor) {
    if (!file_descriptor) return false;

    _address.sin_family = AF_INET;
    _address.sin_addr.s_addr = INADDR_ANY;
    _address.sin_port = htons(_port);

    int status;

    status = bind(file_descriptor, (struct sockaddr*) &_address, sizeof(_address));
    if (status < 0) return false;

    return true;
}

void Server::_doConsumeSocket() {
    int socket_in;
    int address_size = sizeof(_address);

    while (_listen) {
        socket_in = accept(_socket, (struct sockaddr*) &_address, (socklen_t*) &address_size);
        if (socket_in < 0) continue;

        {
            std::lock_guard<std::mutex> lock(_mutex);
            _queue.push_back(socket_in);
            _signal = true;
        }

        _condition.notify_one();
    }
}

void Server::_doProcessSocket(int id) {
    while (_listen) {
        int queue_size = 0;

        {
            std::unique_lock<std::mutex> lock(_mutex);
            _condition.wait(lock,
                [this] {
                    if (this->_signal) return true;
                    if (!this->_listen && !this->_queue.size()) return true;
                    return false;
                }
            );
            queue_size = _queue.size();
        }

        if (!queue_size) {
            {
                std::lock_guard<std::mutex> lock(_mutex);
                _signal = false;
            }

            _condition.notify_one();
            continue;
        }

        int socket_in = 0;
        {
            std::lock_guard<std::mutex> lock(_mutex);
            socket_in = _queue[0];
            _queue.erase(_queue.begin());
        }

        Request* request = new Request(socket_in);
        Response* response = new Response(socket_in);

        int status = _doProcessRequest(request, response);

        delete request;
        delete response;
        close(socket_in);
    }
}

bool Server::_doProcessRequest(Request* request, Response* response) {
    if (!request->isValid()) {
        response->doSendError(HttpStatus::Code::BadRequest, "Invalid request.");
        return false;
    }

    std::string path = request->getPath();
    Struct::Methods method = request->getMethod();

    Routes::Route route;
    if (!(_routes->getRoute(path, method, route) && route.isValid())) {
        response->doSendError(HttpStatus::Code::Forbidden, "Path invalid/not found.");
        return false;
    }

    if (route.method != method) {
        response->doSendError(HttpStatus::Code::MethodNotAllowed, "Method invalid/not found.");
        return false;
    }

    void (*callback)(Request*, Response*) = route.callback;
    callback(request, response);

    if (!response->isSent()) {
        response->doSendError(HttpStatus::Code::ServiceUnavailable, "Resource was not found or can't respond now.");
    }

    return true;
}

Request (parsing) and Response (sending)

Request

#pragma once

#include <unistd.h>
#include <string.h>
#include <iostream>
#include <string>
#include <vector>
#include <unordered_map>
#include <regex>
#include <json.hpp>

#include "Server/Struct.h"

using json = nlohmann::json;

class Request {
    public:
        Request(int socket, unsigned int buffer_size = 1024);
        ~Request();

        bool isValid();
        std::string getPath() {return _attributes.path;}
        Struct::Methods getMethod() {return _attributes.method;}
        std::unordered_map<std::string, std::string> getHeaders() {return _attributes.headers;}
        std::string getHeader(std::string header) {return _attributes.headers[header];}
        json getBody() {return _attributes.body;}

    private:
        int _socket;
        unsigned int _buffer_size;
        std::string _data;
        Struct::Attributes _attributes;
        bool _status;

        std::string _doReceiveData(int sock_in);
        bool _doParseData(std::string data, Struct::Attributes& attributes);
        std::vector<std::string> _doSplitText(std::string text, std::string delimiter);
        std::vector<std::string> _doSplitText(std::string text, std::string delimiter, int lock);
};

Request::Request(int socket, unsigned int buffer_size) {
    _socket = socket;
    _buffer_size = buffer_size;
    _status = false;

    _data = _doReceiveData(_socket);
    if (!_data.length()) return;

    bool result;
    result = _doParseData(_data, _attributes);

    if (!result) return;
    if (!_attributes.isValidRequest()) return;

    _status = true;
}

Request::~Request() {

}

bool Request::isValid() {
    return _status;
}

std::string Request::_doReceiveData(int sock_in) {
    char* buffer = new char[_buffer_size];
    memset(buffer, '\0', _buffer_size);
    read(sock_in, buffer, _buffer_size);

    std::string data;
    data.assign(buffer);
    delete[] buffer;
    return data;
}

bool Request::_doParseData(std::string data, Struct::Attributes& attributes) {
    std::string delimiter = "\r\n";
    std::vector<std::string> rows = _doSplitText(data, delimiter);
    if (!rows.size()) return false;

    std::string header = rows[0];
    rows.erase(rows.begin());

    if (!header.length()) return false;

    std::vector<std::string> parsed_header = _doSplitText(header, std::string(" "));
    if (parsed_header.size() < 2) return false;

    Struct::Methods method = Struct::doParseHttpMethod(parsed_header[0]);
    if (method == Struct::Methods::NONE) return false;

    std::string path = parsed_header[1];

    std::unordered_map<std::string, std::string> headers;
    for (size_t i = 0; i < rows.size(); i++) {
        std::string row = rows[i];
        delimiter = ":";

        std::vector<std::string> splited = _doSplitText(row, delimiter, true);
        if (splited.size() != 2) continue;

        headers[splited[0]] = splited[1];
    }

    _attributes.method = method;
    _attributes.path = path;
    _attributes.headers = headers;

    std::string content_length = headers["Content-Length"];
    int content_size = 0;

    if (content_size = atoi(content_length.c_str())) {
        std::string body = data.substr(data.length() - content_size, data.length());
        json parsed_body = json::parse(body, nullptr, false);
        if (parsed_body != NULL && !parsed_body.is_discarded()) _attributes.body = parsed_body;
    }

    return true;
}

std::vector<std::string> Request::_doSplitText(std::string text, std::string delimiter) {
    std::vector<std::string> result;
    int delimiter_length = delimiter.length();

    std::string block;
    std::string region;
    int index = 0;

    for (size_t i = 0; i < text.length(); i++) {
        block = text.substr(i, delimiter_length);
        if (block.length() != delimiter_length) continue;

        if (block == delimiter) {
            region = text.substr(index, i - index);
            result.push_back(region);
            index = i + delimiter_length;
        }
    }

    return result;
}

std::vector<std::string> Request::_doSplitText(std::string text, std::string delimiter, int lock) {
    ...
}

Response

#pragma once

#include <unistd.h>
#include <iostream>
#include <string>
#include <unordered_map>

#include "Server/Struct.h"

class Response {
    public:
        Response(int socket_in);
        ~Response();

        bool isSent() {return _sent;}
        void setCode(HttpStatus::Code code) {_attributes.code = code;}
        void setHeader(std::string key, std::string value) {_attributes.headers[key] = value;}
        void setBody(json body) {_attributes.body = body;}
        void doClearHeaders() {_attributes.headers.clear();}
        void doClearBody() {_attributes.body = json::value_t::object;}

        bool doSendSuccess();
        bool doSendError(HttpStatus::Code code, const std::string& message);

    private:
        int _socket;
        bool _sent;

        Struct::Attributes _attributes;

        bool _doSendPayload();
        bool _doCreatePayload(std::string& payload);
};

#include "Response.h"

Response::Response(int socket_in) {
    _socket = socket_in;
    _sent = false;
}

Response::~Response() {

}

bool Response::doSendSuccess() {
    setCode(HttpStatus::Code::OK);
    setHeader("Connection", "Closed");
    return _doSendPayload();
}

bool Response::doSendError(HttpStatus::Code code, const std::string& message) {
    setCode(code);
    doClearHeaders();
    doClearBody();

    setHeader("Connection", "Closed");

    json body;
    body["error"] = {};
    body["error"]["code"] = code;
    body["error"]["message"] = message;

    setBody(body);
    return _doSendPayload();
}

bool Response::_doSendPayload() {
    if (_sent) return false;

    int status;

    setHeader("Server", "Dark");
    setHeader("Content-Type", "application/json");

    std::string payload;
    status = _doCreatePayload(payload);
    if (!status) return false;

    status = write(_socket, payload.c_str(), payload.size());
    if (status < 1) return false;

    _sent = true;
    return true;
}

bool Response::_doCreatePayload(std::string& payload) {
    std::string current_payload;
    std::string data = _attributes.body.dump(4);

    int data_length = data.size();

    if (data_length) {
        _attributes.headers["Content-Length"] = std::to_string(data_length);
    }

    current_payload += _attributes.version + " " + std::to_string((int) _attributes.code) + " " + HttpStatus::getReasonPhrase(_attributes.code) + "\r\n";

    std::unordered_map<std::string, std::string>::iterator iterator;
    for (iterator = _attributes.headers.begin(); iterator != _attributes.headers.end(); iterator++){
        std::string key = iterator->first;
        std::string value = iterator->second;

        current_payload += key + ": " + value + "\r\n";
    }

    if (data_length) current_payload += "\r\n" + data + "\r\n\r\n";

    payload = current_payload;
    return true;
}

Routes

#pragma once

#include <iostream>
#include <string>
#include <vector>

#include "Server/Request.h"
#include "Server/Response.h"

class Routes {
    public:
        Routes();
        ~Routes();

        struct Route {
            std::string path;
            Struct::Methods method;
            void (*callback)(Request*, Response*);

            bool isValid() {
                if (!path.length()) return false;
                if (method < Struct::Methods::FIRST || method > Struct::Methods::LAST) return false;
                if (callback == nullptr) return false;
                return true;
            }
        };

        bool setRoute(std::string path, Struct::Methods method, void (*callback)(Request*, Response*));
        bool getRoute(std::string path, Struct::Methods method, Route& route);

    private:
        std::vector<Route> _routes;

        int _getRouteIndex(std::string path, Struct::Methods method);
};

#include "Routes.h"

Routes::Routes() {

}

Routes::~Routes() {

}

bool Routes::setRoute(std::string path, Struct::Methods method, void (*callback)(Request*, Response*)) {
    if (path.length() < 1) return false;
    if (_getRouteIndex(path, method) >= 0) return false;
    if (method < Struct::Methods::FIRST || method > Struct::Methods::LAST) return false;
    if (callback == nullptr) return false;

    Routes::Route route = {path, method, callback};
    _routes.push_back(route);
    return true;
}

bool Routes::getRoute(std::string path, Struct::Methods method, Routes::Route& route) {
    int index = _getRouteIndex(path, method);
    if (index < 0) return false;
    route = _routes[index];
    return true;
}

int Routes::_getRouteIndex(std::string path, Struct::Methods method) {
    for (size_t i = 0; i < _routes.size(); i++) {
        Route* route = &_routes[i];
        if (route->path == path && route->method == method) {
            return i;
        }
    }
    return -1;
}

Struct and StatusCode

StatusCode

http-status-codes-cpp

Struct

#pragma once

#include <json.hpp>

#include "Server/StatusCode.h"

using json = nlohmann::json;

class Struct {
    public:
        enum class Methods {
            NONE = 0, GET = 1, POST = 2, FIRST = GET, LAST = POST
        };

        struct Attributes {
            const std::string version = "HTTP/1.1";

            std::string path;
            Methods method;
            HttpStatus::Code code;
            std::unordered_map<std::string, std::string> headers;
            json body;

            Attributes() {
                code = HttpStatus::Code::InternalServerError;
                body = json::value_t::object;
            }

            bool isValidRequest() {
                if (!path.length()) return false;
                if (method < Methods::FIRST || method > Methods::LAST) return false;
                if (!headers.size()) return false;
                return true;
            }

            bool isValidResponse() {
                if (!headers.size()) return false;
                return true;    
            }
        };

        static Methods doParseHttpMethod(std::string value) {
            Methods target = Methods::NONE;

            if (value == "GET") target = Methods::GET;
            if (value == "POST") target = Methods::POST;

            return target;
        }

    private:
};

Main (usage):

#include "Server/Server.h"

void exec(Request* request, Response* response) {
    json body;

    body["foo"] = 123;
    body["bar"] = true;

    response->setBody(body);
    response->doSendSuccess();
}

int main(int argc, char* argv[]) {
    Server* server = new Server(5000);
    server->setRoute("/getStatus", Struct::Methods::GET, exec);
    server->setRoute("/postStatus", Struct::Methods::POST, exec);
    server->doListen();

    // let threads live for some time before they eternaly be gone
    /*
       actually I'm stuck with this sleep, I don't know how to hold 
       this until caller call doStop() without using while and
       consuming process power
    */
    sleep(30);

    delete server;
    return 1;
}

Compile


#!/bin/bash

sudo g++ \
    main.cpp \
    -I lib \
        ./lib/json.hpp \
    -I src \
        ./src/Server/Server.h ./src/Server/Server.cpp \
        ./src/Server/Request.h ./src/Server/Request.cpp \
        ./src/Server/Response.h ./src/Server/Response.cpp \
        ./src/Server/Routes.h ./src/Server/Routes.cpp \
        ./src/Server/Struct.h \
        ./src/Server/StatusCode.h \
        ./src/Tools/Logger.h \
    -pthread \
-o main.exe

sudo chmod +x main.exe
sudo ./main.exe

Basically I'm mirroring NodeJS express API Usage: server.setRoute(path, route, callback);

So, what can be done to improve my code in terms of optimization and efficiency?

\$\endgroup\$
  • 1
    \$\begingroup\$ Why sudo g++ instead of a simple g++? Why chmod at all? And where is the obligatory -Werror -Wall -Wextra -O2? \$\endgroup\$ – Roland Illig Apr 22 at 0:23
  • \$\begingroup\$ @RolandIllig I don't know man, should I use those things? I always use sudo to avoid those anoying errors, and chmod is to make file executable (this is the way that I learnt -- all from google, including english and code, I don't take classes, so probably I don't know many things). \$\endgroup\$ – Radagast Apr 22 at 0:41
  • \$\begingroup\$ @RolandIllig you can edit if you want, probably I won't know the best approach. \$\endgroup\$ – Radagast Apr 22 at 0:50
8
\$\begingroup\$

Note: This review focuses on the use of C++, rather than the functionality.


Naming:

  • IMHO, the use of "do" at the start of function names is unnecessary and makes the code harder to read. The names would be fine without it (e.g. sendSuccess, sendError, createSocket all make perfect sense).

Server:

  • If the port must always fit in a 16 bit unsigned int, we can use std::uint16_t (from the <cstdint> header) instead of an unsigned int.

  • The new keyword should almost never be used in modern C++. If we need to create it on the heap, _routes should be a std::unique_ptr (from <memory>), which will be cleaned up automatically for us. In this case it looks like the variable could just be created on the stack (i.e. declared as Routes _routes;).

  • _doCreateSocket() returns a bool, but the Server constructor uses an int to hold the return type.

  • It's better to use the constructor's member initializer list to initialize variables where possible (it's neater, and we don't have to worry about initializing objects twice), e.g.:

    Server::Server(std::uint16_t port, unsigned int max_connections, unsigned int thread_count):
        _port(port),
        _max_connections(max_connections),
        _thread_count(thread_count),
        _signal(false),
        _thread_consume(nullptr),
        _thread_process(nullptr),
        _routes(nullptr),
        _socket(-1), // or something
        _listen(false)
    {
        if (max_connections < 1) {
            Logger::doSendMessage(Logger::TYPES::ERROR, "Max connections can't be lower than 1 on Server::Constructor.");
        }
    
        if (!_doCreateSocket(_socket)) {
            Logger::doSendMessage(Logger::TYPES::ERROR, "Failed to create socket on Server::Constructor.");
        }
    
        if (!_doBindSocket(_socket)) {
            Logger::doSendMessage(Logger::TYPES::ERROR, "Failed to bind socket on Server::Constructor.");
        }
    }
    
  • Note that plain data variables (e.g. pointers, ints) are left uninitialized (and may contain any random value) unless we explicitly initialize them. It's safest to always set them to a known value in the constructor.

  • _thread_consume can also be created on the stack (the std::thread default constructor doesn't launch a new thread), and _thread_process can be a std::vector<std::thread>. This saves us from having to do any manual memory management.

  • Prefer to use std::function from the <functional> header, instead of raw function pointers. (e.g. std::function<void(Request*, Response*)>).

  • The request and response variables in Server::_doProcessSocket should be created on the stack. We can still pass them by pointers if necessary by taking their addresses (_doProcessRequest(&request, &response)), or (better) we could pass them by reference.

  • The status code returned by Server::_doProcessRequest() isn't used.

  • In Server::_doProcessRequest(), the if (route.method != method) check is unnecessary, since we used the method while finding the route.

  • The forwarding from doStop to _doStop is unnecessary.

  • This class is doing several things. It manages a thread pool, as well as doing raw socket stuff. We could definitely split the socket functionality into a separate class.


Request:

  • Member functions that don't alter the member variables of a class should be declared const, e.g.: bool isValid() const;. This means we can make proper use of const and const& variables, allowing the compiler to perform better optimisations, and preventing programmer error.

  • The getter functions in this class all return by value. This probably results in some unnecessary copies being made, which may be expensive where the objects require allocation of memory (e.g. copying the unordered_map / strings). It might be better to return by const& instead, e.g.: std::unordered_map<std::string, std::string> const& getHeaders();. This still prevents the caller from altering referenced variable, but allows them to decide whether to copy it, copy part of it, or not copy it at all.

  • In Request::_doReceiveData, we can use a std::vector<char> for the buffer rather than doing manual memory management. (It's guaranteed to provide contiguous memory, which we can access using the .data() member function).


Routes:

  • Routes::setRoute should probably use the Route::isValid method, rather than duplicating the checks.

  • There's some unnecessary string copies in getRoute. We should pass the variable as a reference: const std::string& path instead.

  • Using iterators and the standard library search algorithms is more idiomatic C++ than indices. e.g.:

    auto route = std::find_if(_routes.begin(), routes.end(), 
        [] (Route const& route) { return (route.path == path) && (route->method == method); });
    
    if (route == routes.end()) // route not found!
    
  • (Unless it's reused elsewhere, I'd be inclined to remove the Routes class in favor of a std::vector<Route> _routes; in the Server class.)


Main:

\$\endgroup\$
  • \$\begingroup\$ Thank you for the review! I'm learning so much with that. So, basically, should I avoid most of non-std items? I know that is better to let something handle a situation than you by hand, but in terms of practise and personal development as a programmer, Is it all bad? Probably i will remove routes class and make a Route vector. Thank you so much! I will certainly take your advices. \$\endgroup\$ – Radagast Apr 21 at 2:12
6
\$\begingroup\$

It's not bad code for someone new to C++. Here are some observations and suggestions that may help you improve your code.

Avoid relative paths in #includes

Generally it's better to omit relative path names from #include files and instead point the compiler to the appropriate location. So instead of this:

#include "Server/Routes.h"
#include "Tools/Logger.h"

write this:

#include "Routes.h"
#include "Logger.h"

For gcc, you'd use -I to tell the compiler where to find these files. This makes the code less dependent on the actual file structure, and leaving such details in a single location: a Makefile or compiler configuration file. With cmake, we can use include_directories.

Prefer to avoid using new and delete directly

The server variable within the main() function doesn't really need to be allocated via new. The same is true of the _routes member of Server and probably some other places as well. That way, it is automatically created with the correct length and then discarded when the function is complete or the owning object is deleted.

Avoid leading underscores in names

Anything with a leading underscore is a reserved name in C++ (and in C) in certain scopes. See this question for details.

Prefer modern initializers for constructors

The constructor use the more modern initializer style rather than the old style you're currently using. Instead of this:

Response::Response(int socket_in) {
    _socket = socket_in;
    _sent = false;
}

one could write this:

Response::Response(int socket_in) :
    _socket{socket_in},
    _sent{false}
{}

Let the compiler create default destructor

The compiler will create a destructor by default which is essentially identical to what you've got in several places, so you can simply omit both the declaraton and implementation from your code.

Fix the bug

This is a very subtle bug, but a bug nonetheless. Within Server::setRoute() three threads are created, two of which take this as a parameter. The problem is that there's no guarantee that the object still exists for the duration of the launched threads. What you need is to use enable_shared_from_this with the class and then use a shared_ptr. This question explains a bit more. Generally speaking, it is somewhat difficult to write robust multithreaded code in C++ because there are many ways to create subtle bugs like this.

Avoid needless casts and variables

The current code contains these two lines:

void (*callback)(Request*, Response*) = route.callback;
callback(request, response);

But this could be much more simply written like this:

route.callback(request, response);

Use const where practical

The Request::isValid() and Routes::_getRouteIndex member functions do not alter the underlying objects and therefore should be declared const. That is, the declaration should be:

int _getRouteIndex(std::string path, Struct::Methods method) const;

Use "range for" and simplify your code

The current Routes::_getRouteIndex looks like this:

int Routes::_getRouteIndex(std::string path, Struct::Methods method) {
    for (size_t i = 0; i < _routes.size(); i++) {
        Route* route = &_routes[i];
        if (route->path == path && route->method == method) {
            return i;
        }
    }
    return -1;
}

Using a range for it could be written like this:

int Routes::_getRouteIndex(std::string path, Struct::Methods method) const {
    int i{0};
    for (const auto &r : _routes) {
        if (r.path == path && r.method == method) {
            return i;
        }
        ++i;
    }
    return -1;
}

However, even better in this instance, use the next suggestion.

Use library functions where appropriate

The code for Routes currently stores each Route in a std::vector and then goes through some machinations to recover the index. This could be done more simply by using a std::unordered_map. Also, it may be sufficient just to use the path as an index because the calling code checks that the method matches. Right now, it's not possible to trigger the "Method invalid/not found" error because getRoute only returns true if both the path and the method match.

Omit unused variables

Because argc and argv are unused, you could use the alternative form of main:

int main ()

There are also a number of other places in which passed variables are unused.

\$\endgroup\$
  • 1
    \$\begingroup\$ I disagree with your first item. And your third item is plain incorrect. \$\endgroup\$ – Konrad Rudolph Apr 20 at 23:16
  • \$\begingroup\$ Thank you for the review! I'm using g++ -I to include paths, but I let the folder's name to be more clarifying, am I wrong? I will make some repairs and I will certainly take your advices, thank you so much! \$\endgroup\$ – Radagast Apr 21 at 2:17
  • 1
    \$\begingroup\$ Having paths in include files isn’t wrong necessarily but it makes the code less flexible and harder to maintain because it imposes a directory structure that is harder to change. I find it’s better to put that directory structure in one place (e.g. the Makefile) than spread across every file. \$\endgroup\$ – Edward Apr 21 at 2:45
  • \$\begingroup\$ @Edward Do you also use -I/usr/include/sys so you don't have to write #include <sys/types.h> anymore? In another example, #include <boost/...> clearly says from which project the include file comes. How do you known which of the 50 dependencies provides the <common.h> header? \$\endgroup\$ – Roland Illig Apr 21 at 22:32
  • 1
    \$\begingroup\$ @RolandIllig Project (e.g. boost) and system (e.g. sys) are different because they are well known, widely available and stable. I am referring to files that one writes for one's own particular project. I tend to aggregate logical portions of my projects into either shared or static libraries. I find that this makes them easier to thoroughly unit test. For those reason, If I had a file with 50 dependencies, or a project include file named common.h, I would consider it an design failure or an implementation flaw and would rewrite into more granular, better named pieces. \$\endgroup\$ – Edward Apr 21 at 23:46
3
\$\begingroup\$

When programming in C or C++, there are really many ways to shoot yourself in the foot. To prevent at least some of them, the compilers can produce warnings if they see typical mistakes. But you, as the programmer, have to tell the compiler to give you these warnings.

For GCC, the basic warning level is -Wall. It issues many useful warnings. So instead of running g++ myfile.cpp, run g++ -Wall myfile.cpp.

When you have understood the warnings and have fixed them (in this order), it's time for the next level:

g++ -Wall -O2

The -O2 option enables optimizations. This will make your code run faster, and it also generates new warnings. This is because some of the warnings (like "unused variable") are generated as a byproduct of the optimizations.

The next level is:

g++ -Wall -Wextra -O2

Surprise: the -Wall option doesn't enable all warnings (even if its name suggests so). There are some additional warnings. And even now, you don't get all available warnings.

g++ -Wall -Wextra -pedantic -std=c17

If you are at this stage and have understood and fixed all the warnings, your code is in a good shape. Now try the next compiler, CLang:

clang -Wall -Weverything -O2

If you are developing on Solaris, try SUNPro, if you are developing on IRIX, use MIPSPro.

So far for the compiler.


The chmod command is redundant. A compiler's job is to produces executable files, therefore it will make the file executable. You don't need to do that yourself.

The advice for running chmod +x comes from a different scenario. When you write programs in a programming language where the source code is executed directly, instead of being compiled, there's no program that transforms your written code into machine-executable code. These two kinds of code are in the same file. And only because your text editor does not make the file executable do you have to do this yourself. Examples for such programming languages are Python, Perl, Bash.


You can remove the sudo command completely from your installation program. I'm assuming that all these files are somewhere in your home directory, probably /home/username or /Users/username. In these directories you have all the permissions you need to create files and to execute them.

The sudo command is only needed if you want to do things that affect all users of the computer, such as configuring the wifi or updating the system programs or installing your own programs in /usr/bin.

Don't use sudo unless it is really necessary. When you get error messages, try to understand them.

\$\endgroup\$
  • 2
    \$\begingroup\$ You might also want to suggest that make or CMake are purpose built for the task of compiling programs and are likely better and more portable tools than a bash script for that purpose. \$\endgroup\$ – Edward Apr 22 at 12:58
  • \$\begingroup\$ I did not understand some of those options before, mostly the purpose and usage. Thank you so much for clarifying this for me, certainly it will be so helpful to others as was to me. \$\endgroup\$ – Radagast Apr 22 at 13:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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