7
\$\begingroup\$

I am looking for feedback about a working trading bot (which connects to the API of the deribit website and trades on that platform) I made. I am a beginner in C++ and would like to learn some good practices and design choices that I have unknowingly missed.

CONTEXT: This was started by me as a task for a C++ developer internship a week ago. While I was unable to complete it in the stipulated 4 days (I couldn't even set up a working websocket by that time) I thought it would be fun to take it up as a project. This is my first medium sized project and I am still working on adding more features such as cancel, edit order, etc.

For reference or documentation on how the application is intended to be used, you can see the Github of this repo and the Deribit API documentation.

The libraries used are:

  • websocketpp by zaphoyd: my main file roughly follows this tutorial.
  • fmt for formatting the terminal output
  • nlohmann-json for parsing json objects for sending and receiving to and from the server
  • Boost and OpenSSL to support websocketpp

Here's how the code is roughly structured:

  • The main file is deribot.cpp
  • The user enters some commands in the terminal upon being prompted, and those are processed by the file deribit_api.cpp.
  • The utils file contains some useful and commonly used functions
  • The password.cpp file is a thing I came up with so that I could provide users the option to store their access_token locally in the app for the duration of the current session. The Password class is a singleton and stores the password when the user enters a flag -r during authorization.

deribot.cpp

#include <cstdlib>
#include <iostream>
#include <map>
#include <string>
#include <sstream>
#include <mutex>
#include <condition_variable>

#include "DeriBotConfig.h"
#include "deribit_api.h"
#include "utils.h"
#include "password.h"

#include <websocketpp/config/asio_client.hpp> 
#include <boost/asio.hpp>
#include <boost/asio/ssl.hpp>
#include <boost/asio/ssl/context.hpp> 
#include <websocketpp/client.hpp> 

#include <websocketpp/common/thread.hpp>
#include <websocketpp/common/memory.hpp>

#include <fmt/color.h>

typedef websocketpp::client<websocketpp::config::asio_tls_client> client;
typedef std::shared_ptr<boost::asio::ssl::context> context_ptr;

class connection_metadata {
    private:
        int m_id;
        websocketpp::connection_hdl m_hdl;
        std::string m_status;
        std::string m_uri;
        std::string m_server;
        std::string m_error_reason;
        std::vector<std::string> m_messages;

    public:
        typedef websocketpp::lib::shared_ptr<connection_metadata> ptr;
        typedef std::shared_ptr<boost::asio::ssl::context> context_ptr;

        std::mutex mtx;
        std::condition_variable cv;
        bool msg_processed;

        connection_metadata(int id, websocketpp::connection_hdl hdl, std::string uri):
        m_id(id),
        m_hdl(hdl),
        m_status("Connecting"),
        m_uri(uri),
        m_server("N/A"),
        m_messages({}),
        msg_processed(false)
        {}

        int get_id(){return m_id;}
        websocketpp::connection_hdl get_hdl(){ return m_hdl;}
        std::string get_status(){return m_status;}
        void record_sent_message(std::string message) {
            m_messages.push_back("SENT: " + message);
        }
        
        void on_open(client * c, websocketpp::connection_hdl hdl){
            m_status = "Open";

            client::connection_ptr con = c->get_con_from_hdl(hdl);
            m_server = con->get_response_header("Server");
        }

        void on_fail(client * c, websocketpp::connection_hdl hdl){
            m_status = "Failed";

            client::connection_ptr con = c->get_con_from_hdl(hdl);
            m_server = con->get_response_header("Server");
            m_error_reason = con->get_ec().message();
        }

        void on_close(client * c, websocketpp::connection_hdl hdl){
            m_status = "Closed";

            client::connection_ptr con = c->get_con_from_hdl(hdl);
            std::stringstream s;
            s << "Close code: " << con->get_remote_close_code() << "("
              << websocketpp::close::status::get_string(con->get_remote_close_code())
              << "), Close reason: " << con->get_remote_close_reason();
            
            m_error_reason = s.str();
        }

        void on_message(websocketpp::connection_hdl hdl, client::message_ptr msg) {
            if (msg->get_opcode() == websocketpp::frame::opcode::text) {
                m_messages.push_back("RECEIVED: " + msg->get_payload());
            } else {
                m_messages.push_back("RECEIVED: " + websocketpp::utility::to_hex(msg->get_payload()));
            }

            char show_msg;
            utils::printcmd("Received message. Show message? Y/N ", 57, 255, 20);
            std::cin >> show_msg;
            if(show_msg == 'y' | show_msg == 'Y'){
                if (msg->get_payload()[0] == '{') {
                    std::cout << "Received message: " << utils::pretty(msg->get_payload()) << std::endl;
                }
                else{
                    std::cout << "Received message: " << msg->get_payload() << std::endl;
                }
            }

            if (AUTH_SENT) {
                Password::password().setAccessToken(json::parse(msg->get_payload())["result"]["access_token"]);
                utils::printcmd("Authorization successful!\n");
                AUTH_SENT = false;
            }
            msg_processed = true;
            cv.notify_one();
        }
        
        friend std::ostream &operator<< (std::ostream &out, connection_metadata const &data);
};

std::ostream &operator<< (std::ostream &out, connection_metadata const &data){
    out << "> URI: " << data.m_uri << "\n"
        << "> Status: " << data.m_status << "\n"
        << "> Remote Server: " << (data.m_server.empty() ? "None Specified" : data.m_server) << "\n"
        << "> Error/close reason: " << (data.m_error_reason.empty() ? "N/A" : data.m_error_reason) << "\n"
        << "> Messages Processed: (" << data.m_messages.size() << ") \n";
 
        std::vector<std::string>::const_iterator it;
        for (it = data.m_messages.begin(); it != data.m_messages.end(); ++it) {
            out << *it << "\n";
        }
    return out;
}

context_ptr on_tls_init() {
            context_ptr context = std::make_shared<boost::asio::ssl::context>(boost::asio::ssl::context::sslv23);

            try {
                context->set_options(boost::asio::ssl::context::default_workarounds |
                                boost::asio::ssl::context::no_sslv2 |
                                boost::asio::ssl::context::no_sslv3 |
                                boost::asio::ssl::context::single_dh_use);
            } catch (std::exception &e) {
                std::cout << "Error in context pointer: " << e.what() << std::endl;
            }
            return context;
}

class websocket_endpoint {
    private:
        typedef std::map<int,connection_metadata::ptr> con_list;

        client m_endpoint;
        websocketpp::lib::shared_ptr<websocketpp::lib::thread> m_thread;

        con_list m_connection_list;
        int m_next_id;

    public:
        websocket_endpoint(): m_next_id(0) {
            m_endpoint.clear_access_channels(websocketpp::log::alevel::all);
            m_endpoint.clear_error_channels(websocketpp::log::elevel::all);

            m_endpoint.init_asio();
            m_endpoint.start_perpetual();

            m_thread.reset(new websocketpp::lib::thread(&client::run, &m_endpoint));
        }

        ~websocket_endpoint() {
            m_endpoint.stop_perpetual();
    
            for (con_list::const_iterator it = m_connection_list.begin(); it != m_connection_list.end(); ++it) {
                if (it->second->get_status() != "Open") {
                    continue;
                }
                
                std::cout << "> Closing connection " << it->second->get_id() << std::endl;
                
                websocketpp::lib::error_code ec;
                m_endpoint.close(it->second->get_hdl(), websocketpp::close::status::going_away, "", ec);
                if (ec) {
                    std::cout << "> Error closing connection " << it->second->get_id() << ": "  
                            << ec.message() << std::endl;
                }
            }
            
            m_thread->join();
        }

        int connect (std::string const &uri) {

            int new_id = m_next_id++;

            m_endpoint.set_tls_init_handler(websocketpp::lib::bind(
                                            &on_tls_init
                                            ));

            websocketpp::lib::error_code ec;
            client::connection_ptr con = m_endpoint.get_connection(uri, ec);

            if(ec){
                std::cout << "Connection initialization error: " << ec.message() << std::endl;
                return -1;
            }

            connection_metadata::ptr metadata_ptr(new connection_metadata(new_id, con->get_handle(), uri));
            m_connection_list[new_id] = metadata_ptr;

            con->set_open_handler(websocketpp::lib::bind(
                                  &connection_metadata::on_open,
                                  metadata_ptr,
                                  &m_endpoint,
                                  websocketpp::lib::placeholders::_1
                                  ));

            con->set_fail_handler(websocketpp::lib::bind(
                                  &connection_metadata::on_fail,
                                  metadata_ptr,
                                  &m_endpoint,
                                  websocketpp::lib::placeholders::_1
                                  ));
            con->set_close_handler(websocketpp::lib::bind(
                                   &connection_metadata::on_close,
                                   metadata_ptr,
                                   &m_endpoint,
                                   websocketpp::lib::placeholders::_1
                                  ));
            con->set_message_handler(websocketpp::lib::bind(
                                     &connection_metadata::on_message,
                                     metadata_ptr,
                                     websocketpp::lib::placeholders::_1,
                                     websocketpp::lib::placeholders::_2
                                    ));

            m_endpoint.connect(con);
    
            return new_id;
        }

        connection_metadata::ptr get_metadata(int id) const {
            con_list::const_iterator metadata_it = m_connection_list.find(id);
            if (metadata_it == m_connection_list.end()) {
                return connection_metadata::ptr();
            } else {
                return metadata_it->second;
            }
        }

        void close(int id, websocketpp::close::status::value code, std::string reason) {
            websocketpp::lib::error_code ec;

            con_list::iterator metadata_it = m_connection_list.find(id);
            if (metadata_it == m_connection_list.end()) {
                std::cout << "> No connection found with id " << id << std::endl;
                return;
            }

            m_endpoint.close(metadata_it->second->get_hdl(), code, "", ec);
            std::cout << "Connection closed with reason: " << reason << std::endl;
        }

        void send(int id, std::string message) {
            websocketpp::lib::error_code ec;
        
            con_list::iterator metadata_it = m_connection_list.find(id);

            if (metadata_it == m_connection_list.end()) {
                std::cout << "> No connection found with id " << id << std::endl;
                return;
            }
            
            m_endpoint.send(metadata_it->second->get_hdl(), message, websocketpp::frame::opcode::text, ec);
            if (ec) {
                std::cout << "> Error sending message: " << ec.message() << std::endl;
                return;
            }
            
            metadata_it->second->record_sent_message(message);
        }
};

int main(){
    bool done = false;
    std::string input;
    websocket_endpoint endpoint;

    fmt::print(fg(fmt::color::crimson) | fmt::emphasis::bold,
            "-------------------------- DERIBOT VERSION {}.{} --------------------------\n", 
            DeriBot_VERSION_MAJOR, DeriBot_VERSION_MINOR);
    std::cout << "Type 'help' to check out all available commands\n" << std::endl;
              
    while(!done) {
        fmt::print(fg(fmt::color::cyan) | fmt::emphasis::italic,
            "Enter command: ");
        while (std::getline(std::cin, input) && input.empty()) {}   

        if(input == "quit"){
            done = true;
        } 
        else if(input == "help"){
            std::cout << "\nCOMMAND LIST:\n"
            << "> help: Displays this help text\n"
            << "> quit: exits the program\n"
            << "> connect <URI>: creates a connection with the given URI\n"
            << "> close <id> <code (optional)> <reason (optional)>: closes the connection with the given id with optionally specifiable exit_code and/or reason\n"
            << "> show <id>: Gets the metadata of the connection with the given id\n"
            << "> send <id> <message>: Sends the message to the specified connection\n" << std::endl 
            << "DERIBIT API COMMANDS\n"
            << "> DERIBIT connect: Directly connects to the Deribit testnet website\n"
            << "> DERIBIT authorize <id> <client_id> <client_secret>: sends the authorization message to retrieve the access token\n\tAn optional flag -r can be set to remember the access_token for the rest of the session"
            << "> DERIBIT buy <id> <instrument> <comments>: Sends a buy order via the connection with id <id> for the instrument specified\n"
            << "> DERIBIT sell <id> <instrument> <comments>: Sends a sell order via the connection with id <id> for the instrument specified\n"
            << std::endl;
        }
        else if (input.substr(0,7) == "connect") {
            int id = endpoint.connect(input.substr(8));
            if (id != -1) {
                std::cout << "> Created connection with id " << id << std::endl;
            }
        } 
        else if (input.substr(0,4) == "show") {
            int id = atoi(input.substr(5).c_str());
 
            connection_metadata::ptr metadata = endpoint.get_metadata(id);
            if (metadata) {
                std::cout << *metadata << std::endl;
            } else {
                std::cout << "> Unknown connection id " << id << std::endl;
            }
        }
        else if(input.substr(0, 5) == "close"){
            std::stringstream ss(input);

            std::string cmd;
            int id;
            int close_code = websocketpp::close::status::normal;
            std::string reason;

            ss >> cmd >> id >> close_code;
            std::getline(ss, reason);

            endpoint.close(id, close_code, reason);
        }
        else if (input.substr(0,4) == "send") {
            std::stringstream ss(input);
                
            std::string cmd;
            int id;
            std::string message = "";
            
            ss >> cmd >> id;
            std::getline(ss,message);
            
            endpoint.send(id, message);

            std::unique_lock<std::mutex> lock(endpoint.get_metadata(id)->mtx);
            endpoint.get_metadata(id)->cv.wait(lock, [&] { return endpoint.get_metadata(id)->msg_processed;});
            endpoint.get_metadata(id)->msg_processed = false;
        }
        else if (input == "DERIBIT connect") {
            std::string uri = "wss://test.deribit.com/ws/api/v2";
            int id = endpoint.connect(uri);
            if (id != -1) {
                std::cout << "> Created connection to DERIBIT TESTNET with id " << id << std::endl;
            }
        }
        else if (input.substr(0, 7) == "DERIBIT") {
            int id; 
            std::string cmd;

            std::stringstream ss(input);
            ss >> cmd >> id;
            
            std::string msg = deribit_api::process(input);
            if (msg != ""){
                endpoint.send(id, msg);
                std::unique_lock<std::mutex> lock(endpoint.get_metadata(id)->mtx);
                endpoint.get_metadata(id)->cv.wait(lock, [&] { return endpoint.get_metadata(id)->msg_processed;});
                endpoint.get_metadata(id)->msg_processed = false;
            }
        }
        else{
            std::cout << "Unrecognized command" << std::endl;
        }
    }
    return 0;
}

deribit_api.cpp

#include "deribit_api.h"
#include "utils.h"
#include "json.hpp"
#include "password.h"

#include <iostream>
#include <string>
#include <sstream>

using json = nlohmann::json;
bool AUTH_SENT = false;

std::string deribit_api::process(const std::string &input) {

    std::istringstream s(input.substr(8));
    std::string cmd;
    s >> cmd;
    
    if (cmd == "authorize") {
        return deribit_api::authorize(input.substr(8));
    }
    else if (cmd == "sell") {
        return deribit_api::sell(input.substr(8));
    }
    else if (cmd == "buy") {
        return deribit_api::buy(input.substr(8));
    }
    else{
        utils::printerr("ERROR: Unrecognized command. Please enter 'help' to see available commands.\n");
        return "";
    }
}

std::string deribit_api::authorize(const std::string &input) {

    std::istringstream s(input);
    std::string auth;
    std::string id;
    std::string flag{""};
    std::string client_id;
    std::string secret;
    long long tm = utils::time_now();

    s >> auth >> id >> client_id >> secret >> flag;
    std::string nonce = utils::gen_random(10);
    std::string signature = utils::get_signature(tm, nonce, "", secret);

    jsonrpc j;
    j["method"] = "public/auth";
    j["params"] = {{"grant_type", "client_signature"}, 
                   {"client_id", client_id},
                   {"timestamp", tm},
                   {"signature", signature},
                   {"nonce", nonce},
                   {"scope", "connection"}
                   };
    if (flag == "-r" && j.dump() != "") AUTH_SENT = true;
    return j.dump();
}

std::string deribit_api::sell(const std::string &input) {
    std::string sell;
    std::string id;
    std::string instrument;
    std::string cmd;
    std::string access_key;
    std::string order_type;
    std::string label;
    std::string frc;
    int contracts{0};
    int amount{0};
    int price{0};

    std::istringstream s(input);
    s >> sell >> id >> instrument >> label;

    if (Password::password().getAccessToken() == "") {
        utils::printcmd("Enter the access token: ");
        std::cin >> access_key;
    }
    else {
        access_key = Password::password().getAccessToken();
    }

    utils::printcmd("\nEnter the amount or contracts: ");
    std::cin >> cmd;
    if (cmd == "contracts") {
        std::cin >> contracts;
    }
    else if (cmd == "amount") {
        std::cin >> amount;
    }
    else {
        utils::printerr("\nIncorrect syntax; couldn't place order\n");
        return "";
    }

    utils::printcmd("Enter the order type: ");
    std::cin >> order_type;
    std::vector<std::string> permitted_order_types = {"limit", "stop_limit", "take_limit", "market", "stop_market", "take_market", "market_limit", "trailing_stop"};

    if (!std::any_of(permitted_order_types.begin(), permitted_order_types.end(), [&](std::string val){ return val == order_type; }))
    {
        utils::printerr("\nIncorrect syntax; couldn't place order\nOrder type can be only one of \nlimit\nstop_limit\ntake_limit\nmarket\nstop_market\ntake_market\nmarket_limit\ntrailing_stop\n");
        return "";
    }
    else if (order_type == "limit" || order_type == "stop_limit") {
        utils::printcmd("\nEnter the price at which you want to sell: ");
        std::cin >> price;
    }

    utils::printcmd("Enter the time-in-force value: ");
    std::cin >> frc;

    std::vector<std::string> permitted_tif = {"good_til_cancelled", "good_til_day", "fill_or_kill", "immediate_or_cancel"};
    if (!std::any_of(permitted_tif.begin(), permitted_tif.end(), [&](std::string val){ return val == frc; }))
    {
        utils::printerr("\nIncorrect syntax; couldn't place order\nTime-in-force value can be only one of \ngood_til_cancelled\ngood_til_day\nfill_or_kill\nimmediate_or_cancel\n");
        return "";
    } 

    jsonrpc j;
    
    j["method"] = "private/sell";
    j["params"] = {{"instrument_name", instrument},
                   {"access_token", access_key}};
    if (amount) { 
        j["params"]["amount"] = amount;
    }
    else {
        j["params"]["contracts"] = contracts;
    }
    j["params"]["type"] = order_type;
    j["params"]["label"] = label;
    j["params"]["time_in_force"] = frc;

    return j.dump();
}

std::string deribit_api::buy(const std::string &input) {
    std::string buy;
    std::string id;
    std::string instrument;
    std::string cmd;
    std::string access_key;
    std::string order_type;
    std::string label;
    std::string frc;
    int contracts{0};
    int amount{0};
    int price{0};

    std::istringstream s(input);
    s >> buy >> id >> instrument >> label;

    if (Password::password().getAccessToken() == "") {
        utils::printcmd("Enter the access token: ");
        std::cin >> access_key;
    }
    else {
        access_key = Password::password().getAccessToken();
    }

    utils::printcmd("\nEnter the amount or contracts: ");
    std::cin >> cmd;
    if (cmd == "contracts") {
        std::cin >> contracts;
    }
    else if (cmd == "amount") {
        std::cin >> amount;
    }
    else {
        utils::printerr("\nIncorrect syntax; couldn't place order\n");
        return "";
    }

    utils::printcmd("Enter the order type: ");
    std::cin >> order_type;
    std::vector<std::string> permitted_order_types = {"limit", "stop_limit", "take_limit", "market", "stop_market", "take_market", "market_limit", "trailing_stop"};

    if (!std::any_of(permitted_order_types.begin(), permitted_order_types.end(), [&](std::string val){ return val == order_type; }))
    {
        utils::printerr("\nIncorrect syntax; couldn't place order\nOrder type can be only one of \nlimit\nstop_limit\ntake_limit\nmarket\nstop_market\ntake_market\nmarket_limit\ntrailing_stop\n");
        return "";
    }
    else if (order_type == "limit" || order_type == "stop_limit") {
        utils::printcmd("\nEnter the price at which you want to sell: ");
        std::cin >> price;
    }

    utils::printcmd("Enter the time-in-force value: ");
    std::cin >> frc;

    std::vector<std::string> permitted_tif = {"good_til_cancelled", "good_til_day", "fill_or_kill", "immediate_or_cancel"};
    if (!std::any_of(permitted_tif.begin(), permitted_tif.end(), [&](std::string val){ return val == frc; }))
    {
        utils::printerr("\nIncorrect syntax; couldn't place order\nTime-in-force value can be only one of \ngood_til_cancelled\ngood_til_day\nfill_or_kill\nimmediate_or_cancel\n");
        return "";
    } 

    jsonrpc j;
    
    j["method"] = "private/buy";
    j["params"] = {{"instrument_name", instrument},
                   {"access_token", access_key}};
    if (amount) { 
        j["params"]["amount"] = amount;
    }
    else {
        j["params"]["contracts"] = contracts;
    }
    j["params"]["type"] = order_type;
    j["params"]["label"] = label;
    j["params"]["time_in_force"] = frc;

    return j.dump();
}

deribit_api.h

#pragma once

#include "json.hpp"
#include <iostream>
#include <string>

using json = nlohmann::json;
extern bool AUTH_SENT;

class jsonrpc : public json {
    public:
        jsonrpc(){
            (*this)["jsonrpc"] = "2.0",

            srand( time(NULL) );
            int number = rand();
            (*this)["id"] = number;
        }
};

namespace deribit_api {

    std::string process(const std::string &input);

    std::string authorize(const std::string &cmd);

    std::string sell(const std::string &input);

    std::string buy(const std::string &input);
}

utils.cpp

#include <iostream>
#include <sstream>
#include <iomanip>
#include "utils.h"

#include <chrono>
#include <time.h>
#include <openssl/hmac.h>
#include <openssl/evp.h>
#include "json.hpp"

#ifdef _WIN32
#include <conio.h> 
#else
#include <termios.h>
#include <unistd.h>
#endif

using json = nlohmann::json;

void utils::printcmd(std::string const &str){
    fmt::print(fg(fmt::rgb(219, 186, 221)), str);
}

void utils::printcmd(std::string const &str, int r, int g, int b){
    fmt::print(fg(fmt::rgb(r, g, b)), str);
}

void utils::printerr(std::string const &str){
    fmt::print(fg(fmt::rgb(255, 83, 29)) | fmt::emphasis::bold, str);
}

long long utils::time_now(){
    auto now = std::chrono::system_clock::now();
    auto now_ms = std::chrono::time_point_cast<std::chrono::milliseconds>(now);
    auto epoch = now_ms.time_since_epoch();
    long long unix_timestamp_ms = epoch.count();

    return unix_timestamp_ms;
}

std::string utils::gen_random(const int len) {
    static const char alphanum[] =
        "0123456789"
        "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
        "abcdefghijklmnopqrstuvwxyz";
    std::string tmp_s;
    tmp_s.reserve(len);

    for (int i = 0; i < len; ++i) {
        tmp_s += alphanum[rand() % (sizeof(alphanum) - 1)];
    }
    
    return tmp_s;
}

std::string utils::to_hex_string(const unsigned char* data, unsigned int length) {
    std::ostringstream hex_stream;
    hex_stream << std::hex << std::uppercase << std::setfill('0');
    for (unsigned int i = 0; i < length; ++i) {
        hex_stream << std::setw(2) << static_cast<int>(data[i]);
    }
    return hex_stream.str();
}

std::string utils::hmac_sha256(const std::string& key, const std::string& data) {
    unsigned char result[EVP_MAX_MD_SIZE];
    unsigned int result_length = 0;

    HMAC(EVP_sha256(), key.c_str(), key.length(),
         reinterpret_cast<const unsigned char*>(data.c_str()), data.length(),
         result, &result_length);

    return utils::to_hex_string(result, result_length);
}

std::string utils::get_signature(long long timestamp, std::string nonce, std::string data, std::string clientsecret){

    std::string string_to_code = std::to_string(timestamp) + "\n" + nonce + "\n" + data;
    return utils::hmac_sha256(clientsecret, string_to_code);
}

std::string utils::pretty(std::string j) {
    json serialised = json::parse(j);
    return serialised.dump(4);
}

std::string utils::getPassword() {
    std::string password;
    char ch;

    std::cout << "Enter access key: ";

    #ifdef _WIN32
        while ((ch = _getch()) != '\r') { 
            if (ch == '\b') {  
                if (!password.empty()) {
                    password.pop_back();
                    std::cout << "\b \b";
                }
            } else {
                password += ch;
                std::cout << '*'; 
            }
        }
        std::cout << std::endl;

    #else
        struct termios oldt, newt;
        tcgetattr(STDIN_FILENO, &oldt); 
        newt = oldt;
        newt.c_lflag &= ~ECHO;
        tcsetattr(STDIN_FILENO, TCSANOW, &newt);

        std::cin >> password;

        tcsetattr(STDIN_FILENO, TCSANOW, &oldt);
        std::cout << std::endl;
    #endif

    return password;
}

utils.h

#pragma once

#include <iostream>
#include <fmt/color.h>

namespace utils {

    long long time_now();

    std::string gen_random(const int len);
    
    std::string get_signature(long long timestamp, std::string nonce, std::string data, std::string clientsecret);

    std::string to_hex_string(const unsigned char* data, unsigned int length);

    std::string hmac_sha256(const std::string& key, const std::string& data);

    std::string pretty(std::string j);

    std::string getPassword();

    void printcmd(std::string const &str);
    void printcmd(std::string const &str, int r, int g, int b);
    void printerr(std::string const &str);
}

password.cpp

#include <iostream>
#include <string>
#include "password.h"

Password *Password::pwd = nullptr;
int Password::num_sets = 0;  

Password &Password::password() {
    if (!pwd) {
        pwd = new Password();
    }
    return *(pwd);
}

void Password::setAccessToken(const std::string& token) {
    if(num_sets > 1) {
        std::cout << "WARNING: Access token can only be set once per session" << std::endl;
        return;
    }
    access_token = token;
    num_sets++;
}

void Password::setAccessToken(int& token) {
    if(num_sets > 1) {
        std::cout << "WARNING: Access token can only be set once per session" << std::endl;
        return;
    }
    access_token = std::to_string(token);
    num_sets++;
}

std::string Password::getAccessToken() const {
    return access_token;
}

password.h

#pragma once

#include <iostream>
#include <string>

class Password {

    private:
        static Password* pwd;
        static int num_sets;
        std::string access_token;
        
        Password() : access_token("") {}

    public:
        static Password &password();

        Password(const Password&) = delete;
        void operator=(const Password&) = delete;

        void setAccessToken(const std::string& token);
        void setAccessToken(int& token);

        std::string getAccessToken() const;
};

What kind of response am I requesting?

Feedback like

  1. "you designed this part like so and so, but this is inefficient in such and such scenario and you should DEFINITELY change your code to make it more maintainable."
  2. "You made a mistake in this part and even though your program is runnig fine now, it is bound to crash sometime because of so and so reason"
  3. "You missed using this design principle to make your code better when there was a great usecase here/You are using this design wrong"
  4. "You used the "wrong-in-this-scenario" syntax, here's a much better feature C++ provides that you should use instead."

As you can see, I have got around to using only classes till now, templates and ABCs and stuff like that do not come intuitively to me since I am moving to CPP from Python.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ That is a lot of code in one post. \$\endgroup\$ Commented Nov 6 at 17:08
  • \$\begingroup\$ Hi @LokiAstari! What would be a viable solution in this case? I am new to this site so would love to get any potential solutions to this problem. \$\endgroup\$ Commented Nov 6 at 17:24
  • 1
    \$\begingroup\$ People will still review. But there is just a lot to go through in one sitting. I did Password class. I may come back when I have some time to look at others. \$\endgroup\$ Commented Nov 6 at 17:42
  • \$\begingroup\$ C++ Json library comparison. lokiastari.com/Json/Performance.osx.html \$\endgroup\$ Commented Nov 6 at 18:51

3 Answers 3

8
\$\begingroup\$

The API class:

There are two things I would change here.

1: Use the command pattern. 2: Don't pass strings around as actions.

The Command pattern

This is basically where you set up a container with a set of actions keyed on some value (in this case a string, but it could be anything). You then search the container for the command and execute it. A very simple example is a calculator.

std::map<std::string, std::function<int(int, int)>>  actionMap =
{
    {"add", [](int l, int r){return l + r;}},
    {"sub", [](int l, int r){return l - r;}},
    {"mul", [](int l, int r){return l * r;}},
    {"div", [](int l, int r){return l / r;}}
}

int action(std::string const& command, int l, int r)
{
    auto find = actionMap.find(command);
    if (find == actionMap.end()) {
        throw std::runtime_error("Invalid command"); 
    }
    return find->second(l, r);
}

I think you can make your code simpler by applying this pattern to your "process()" function.


Don't pass strings around as actions.

The functions in this interface return a string.

std::string authorize(const std::string &cmd);
std::string sell(const std::string &input);
std::string buy(const std::string &input);

Which I bet at some point gets written to a socket. So you have serialized the command to a string. Copied that string around then copied to the string to socket buffer before eventually the OS flushes the buffer.

I think there is an opertunity for a Serialization interface and a command object.

I would change this interface to return a "Command" (as a std::unique_ptr) that has three implementations AuthirizeCommand, SellCommand and BuyCommand. At the point these objects are passed to your web socket code that is the point where these objects serialize themsleves to the socket.

 class Command
 {
     public:
         virtual ~Command() {}
         virtual void send(SocketInterface&) = 0;
 };
 class AuthirizeCommand
 {
     // All the information needed for authorization.
     public:
          virtual void send(SocketInterface& s) override
          {
               // Write a JSON object to the socket s.
          }
 };
 // etc.


std::unique_ptr<Command> authorize(const std::string &cmd);
std::unique_ptr<Command> sell(const std::string &input);
std::unique_ptr<Command> buy(const std::string &input);

This way you don't serialize the object into a string before you need to. If you decide you are not going to send the object then no wasted processing. It is also very cheap to pass a std::unique_ptr around so not lost processing.

\$\endgroup\$
2
  • \$\begingroup\$ Hi, I'm so sorry for replying so late, thing is I had my semester exams so couldn't work on this. I implemented the first part of this answer and I was amazed by how much nicer it looks! \$\endgroup\$ Commented Nov 23 at 5:46
  • \$\begingroup\$ I'll do the other parts and let you know soon. \$\endgroup\$ Commented Nov 23 at 5:48
7
\$\begingroup\$

Looking at the "Password" class.

You are using the singleton pattern. But not well.

This:

Password &Password::password() {
    if (!pwd) {
        pwd = new Password();
    }
    return *(pwd);
}

Can be better written as:

Password& Password::password()
{
    // notice the "static" keyword.
    static Password pwd;

    // This makes sure the object lives beyond the end of the function.
    // It will be created the first time the function is called and
    // automatically destroyed at the end of the program.


    // Here you are returning a reference to the internal object.
    return pwd;
}

Setting the access token:

You are forcing a copy of the access token string (which may not be needed) when setting and the get copies the access token (when you can return a reference).

        // If your "token" is a R-Value
        // this will work but have an unnecessary copy.
        void setAccessToken(const std::string& token);
        // Why pass by reference. Not sure why you have
        // this interface. Just have a string interface.
        // Any type can be converted string.
        void setAccessToken(int& token);

        // Here we are copying the token out of the object.
        // This may not be necessary.
        std::string getAccessToken() const;

I would write the interface like this:

          void setAccessToken(std::string token);
          std::string const& getAccessToken() const;

I know passing the token by value seems unintuitively wrong (as you seem to be forcing a copy). But bare with me.

  // By accepting by value
  // It automatically works with both
  // Standard const references and with R value references
  // i.e. This is a short cut to having two version of this func:
  //     void Password::setAccessToken(std::string const& token)
  //     void Password::setAccessToken(std::string&& token)
  void Password::setAccessToken(std::string token)
  {
       // Here we move the string.
       // Thus preventing a copy.
       access_token = std::move(token);
  }

Now if the code is used like this:

  Password::password().setAccessToken("Token");

Here we create the std::string once and then move it into the object. While the original version would create the string at the call point then create another copy when you copy the parameter into the object access_token variable.

Then the get access token is simply:

  std::string const& Password::getAccessToken() const {return access_token;}

Generating warnings:

    if(num_sets > 1) {
        std::cout << "WARNING: Access token can only be set once per session" << std::endl;
        return;
    }
  • If these are errors then std::cerr is a better stream.
  • Don't use std::endl it is usually the wrong choice.
  • I would stop the application at this point. This is a programming error. Throw an exception.
\$\endgroup\$
1
  • \$\begingroup\$ It's often necessary for singletons to be able to destroy (and force re-creation) at arbitrary moments in the lifetime, especially when unit testing. So a static object lifetime is often unsuitable for that and you will usually end up with code similar to the original. Though using std::unique_ptr would be better. \$\endgroup\$
    – Miral
    Commented Nov 7 at 5:04
5
\$\begingroup\$

Looking the namespace Utils.

Code Review

#include <iostream>
#include <sstream>
#include <iomanip>
#include "utils.h"       // Don't like putting local headers
                         // in the middle of standard headers.
                     
#include <chrono>
#include <time.h>

Not a fan of this version of JSON parsing. But its OK.

using json = nlohmann::json;

PS. I am joking (I am the author of another JSON parsing library). This one is totally great. Its just not mine :-)


The Print Cmd:

void utils::printcmd(std::string const &str){
    fmt::print(fg(fmt::rgb(219, 186, 221)), str);
}

void utils::printcmd(std::string const &str, int r, int g, int b){
    fmt::print(fg(fmt::rgb(r, g, b)), str);
}

void utils::printerr(std::string const &str){
    fmt::print(fg(fmt::rgb(255, 83, 29)) | fmt::emphasis::bold, str);
}

I would use function overloading to reduce these down a bit. Also rather than using magic numbers fmt::rgb(255, 83, 29) and fmt::rgb(219, 186, 221) I would create some named values. Also why are you passing the parameters as three different int r, int g, int b values when there is already an object that represents a color fmt::rgb?

Why not

static const fmt::rgb  normalColor{219, 186, 221}; // pick a better name!
static const fmt::rgb  errorColor{255, 83, 29};    // Needs better name,

// Roll the first two print functions into a single function with
// default parameters.
void utils::printcmd(std::string const &str, fmt::rgb color = normalColor)
{
    fmt::print(fg(color), str);
}

void utils::printerr(std::string const &str){
    fmt::print(fg(errorColor) | fmt::emphasis::bold, str);
}

Users can the us it the same as before. But when they want to speicy a color they can that simply.

printcmd("MyCommand");             // normal color.
printcmd("RedCommand", {255,0,0}); // prints red.

Not a fan of using long long here.

long long utils::time_now()

I would use a named type. But this may be tightly coupled to your main code. So I am not complaining to much.


Random number generation.

There are lots of example on SO on how to generate random numbers. Please stop using rand() Its a known problem on how un-random it is. Also the modulo method does not give an even distribution (unless sizeof(alphanum) - 1 is an exact multiple of RANDMAX. This is a C function left over from the old days, C++ has its own random number library.

std::string utils::gen_random(const int len)

 /// Stuff removed

        tmp_s += alphanum[rand() % (sizeof(alphanum) - 1)];

 ///  Stuff removed.
}

See: https://stackoverflow.com/a/13445752/14065


You use pointers a lot. This is a no-no in C++. There is always a better alternative to a pointer.

std::string utils::to_hex_string(const unsigned char* data, unsigned int length);

The trouble with pointers is that ownership is not defined so you don't know if you should release the memory. So to expose ownership we use smart pointers and containers and to show non ownership we use views or references (and in the few situations where you pass nullptr to indicate no value we would use std::optional).

In this case it is clear that you are not passing ownership so you should pass a view onto the data.

std::string utils::to_hex_string(std::string_view data)
{
    std::ostringstream hex_stream;
    hex_stream << std::hex << std::uppercase << std::setfill('0');

    for (auto const& val: data) {
        hex_stream << std::setw(2) << static_cast<int>(data[i]);
    }

    return hex_stream.str();
}

std::string_view is a very light wait object (so can be passed by value) and is used to point at some existing text.

Usage:

     return utils::to_hex_string({result, result + result_length});

When passing string around by careful about passing by value.

std::string utils::get_signature(long long timestamp, std::string nonce, std::string data, std::string clientsecret);

When you pass by value you are copying the string. When being passed through several layers of API like this you are makeing a copy of the string on each call. In this situation it is best t pass by const reference.

std::string utils::get_signature(long long timestamp, std::string const& nonce, std::string const& data, std::string const& clientsecret);

NOTE: Passing by value is good when used with move semantics to pass ownership of the string to some internally stored structure (like I showed in the password example).


\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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