3
\$\begingroup\$

Hi :) I am a beginner coder who just learnt C++ and I wanted to apply it in projects to solidify my learning process. However, I only took a short tutorial before trying this out hence I'm pretty unsure about my C++ coding practices and I would like to know what areas I can improve on - and perhaps what kind of resources are available to do so. Additionally, I do feel that my project is a little sparse as of now so I would like suggestions on any features that I could add in to challenge my learning process and also make the bot a little cooler? Thanks for your thoughts in advance!

Context
I used the API 'tgbot' created for coding Telegram Bots using C++ here: https://github.com/reo7sp/tgbot-cpp.
I coded and ran this program a Macbook Air on OSX 10.15 Catalina.

Main Overview of the Bot
The user starts the bot function using '/start' - the bot prompts the user and this brings us to the main interface where:

  • The program accepts a string from the user which details any troubles or any general concerns the user has regarding life
  • Parses through to check for keywords that indicate a certain genre/topic of quotes
  • If a topic is identified: a random quote from the topic will be returned

After this, the a feedback interface is initialised and the user is asked if the quote was useful

  • Some form of yes would redirect the user to the main interface
  • An ambiguous answer not detected to be either yes or no would prompt the user to key in only either one
  • Some form of no would allow the user to choose the reason of this: Was it the correct topic and wrong quote or the wrong topic?

Wrong Topic: leads user to an interface of inline buttons where they can choose the relevant topic. Wrong Quote: would simply generate another random quote of the same topic.

Note: In some of my main.cpp and my other files I may have mixed in the method of 'using namespace std;' and just using 'std::' wherever required because halfway through I heard the latter is better practice XD

Below is the main code with all the header and other source files.

main.cpp

#include <string>
#include <random>
#include <fstream>
#include <vector>
#include <stdexcept>
#include <sstream>
#include <tgbot/tgbot.h>
#include <iostream>
#include "QuoteData.hpp"
#include "Functions.hpp"
using namespace std;
using namespace TgBot;

int main() {
    //Setting up API
    string token(getenv("TOKEN"));
    cout << "Token: " << token.c_str() << endl;

    //Telegram Bot object  created using API token
    Bot bot(token);

    //Declaring/initializing important variables
    std::vector<std::shared_ptr<QuoteTopic>> AllTopics = store_QuoteTopics();
    // flag controls the segment of the chat we are in.
    int flag = 1;
    int TopicIndex;
    std::string Return_Quote;
    std::vector<string> favourites;

    //Start page of the bot
    bot.getEvents().onCommand("start", [&bot, &flag](const Message::Ptr& message) {
        bot.getApi().sendMessage(message->chat->id,
                "Hello, I'm InspoBot. I trade in your worries with a good inspirational quote to help you start the day right. "
                "Think of me as a friend and type in whatever's been worrying you lately."
                "\n \nYou can also key in the command '/topics' to choose from our list of quote topics." );
        flag = 1;
    });

    //To look at and store favourites
    bot.getEvents().onCommand("favourites", [&bot, &favourites](const Message::Ptr& message){
        if (favourites.empty())
            bot.getApi().sendMessage(message->chat->id, "Whoops, looks like you haven't saved any favourites yet. \n"
                                                        "First, find a quote by typing in what's been bothering you, or using the command"
                                                        " '/topics' to choose from our list of quote topics.");
        else{
            for (int n=0;n<favourites.size();n++){
                bot.getApi().sendMessage(message->chat->id, favourites[n] + "\n \n");
            }
        }
    });

    //Creating Inline Keyboards: (Formatting and Respective Queries sent)
    // 1. Quote Keyboard Configurations
    InlineKeyboardMarkup::Ptr keyboard(new InlineKeyboardMarkup);
    vector<InlineKeyboardButton::Ptr> topic_row;
    //Creating Buttons
    for(int n=0; n<AllTopics.size(); n++){
        topic_row.clear();
        InlineKeyboardButton::Ptr KeyboardButton (new InlineKeyboardButton);
        KeyboardButton->text = AllTopics[n]->Topic;
        KeyboardButton->callbackData = to_string(n);
        topic_row.push_back(KeyboardButton);
        keyboard->inlineKeyboard.push_back(topic_row);
    }

    //Manual method of access
    bot.getEvents().onCommand("topics", [&bot, &keyboard] (const Message::Ptr& message) {
        bot.getApi().sendMessage(message->chat->id, "Buttons."
                ,false, 0, keyboard, "Markdown");
    });

    //2. Choice Keyboard Configurations
    InlineKeyboardMarkup::Ptr choice(new InlineKeyboardMarkup);
    vector<InlineKeyboardButton::Ptr> choice_row;
    //Creating Buttons
    InlineKeyboardButton::Ptr KeyboardButton_1 (new InlineKeyboardButton);
    KeyboardButton_1->text = "Wrong Subject";
    KeyboardButton_1->callbackData = "Change Subject";
    choice_row.push_back(KeyboardButton_1);

    InlineKeyboardButton::Ptr KeyboardButton_2 (new InlineKeyboardButton);
    KeyboardButton_2->text = "Wrong Quote";
    KeyboardButton_2->callbackData = "Change Quote";
    choice_row.push_back(KeyboardButton_2);

    choice->inlineKeyboard.push_back(choice_row);

    // Actions to be executed depending on what queries are sent
    bot.getEvents().onCallbackQuery([&bot, &AllTopics, &flag, &TopicIndex, &keyboard, &Return_Quote] (const CallbackQuery::Ptr& query) {
        if (query->data.find("Change")!=std::string::npos){
            if (query->data=="Change Subject") {
                bot.getApi().sendMessage(query->message->chat->id,
                                         "Oh no. Here's a few buttons you can choose between to indicate your area of concern instead.",
                                         false, 0, keyboard, "Markdown");
            }
            else{
                Return_Quote = AllTopics[TopicIndex]->generate_quote();
                bot.getApi().sendMessage(query->message->chat->id, "Here's another quote just for you: ");
                bot.getApi().sendMessage(query->message->chat->id, Return_Quote + "\n");
                bot.getApi().sendMessage(query->message->chat->id, "Was the quote useful for you?");
                flag = 2;
            }
        }
        else {
            Return_Quote = AllTopics[stoi(query->data)]->generate_quote();
            bot.getApi().sendMessage(query->message->chat->id, "Here's a quote just for you: ");
            bot.getApi().sendMessage(query->message->chat->id, Return_Quote + "\n");
            bot.getApi().sendMessage(query->message->chat->id, "Was this quote useful for you?");
            flag = 2;

        }
    });

    //Main Telegram Logic
    bot.getEvents().onAnyMessage([&bot, &AllTopics, &flag, &choice, &TopicIndex, &favourites, &Return_Quote](const Message::Ptr &message) {
        printf("User wrote %s\n", message->text.c_str());
        if (StringTools::startsWith(message->text, "/start") || (message->text == "/end") ||
            (message->text == "/topics")|| (message->text == "/favourites")) {
            return;
        }
        //Main Chat
        if (flag == 1) {
            TopicIndex = which_topic(message->text, AllTopics);
            if (TopicIndex == -1) {
                bot.getApi().sendMessage(message->chat->id,
                                         "Sorry, I couldn't quite understand you. Would you like to try again?");
                return;
            } else {
                Return_Quote = AllTopics[TopicIndex]->generate_quote();
                bot.getApi().sendMessage(message->chat->id, "Here's a quote just for you: ");
                bot.getApi().sendMessage(message->chat->id, Return_Quote + "\n");
                bot.getApi().sendMessage(message->chat->id, "Was the quote useful for you?");
                flag = 2;
                return;
            }
        }
        //Feedback Chat
        if (flag == 2) {
            if (check_yes(message->text)) {
                bot.getApi().sendMessage(message->chat->id,
                                         "That's great! Would you like to store the quote in your list of favourites?");
                flag = 3;
                return;
            } else if (check_no(message->text)) {
                bot.getApi().sendMessage(message->chat->id, "Oh no. Why so?", false, 0, choice, "Markdown");
                return;
            } else
                bot.getApi().sendMessage(message->chat->id,"Well that's confusing... :0 \nHelp me out here and key in a simple 'yes' or 'no' please.");
        }
        if (flag == 3) {
            if (check_yes(message->text)) {
                favourites.push_back(Return_Quote);
                std::cout << Return_Quote << std::endl;
                bot.getApi().sendMessage(message->chat->id, "Okay stored! ;)\nYou can view your list of favourites by typing in the command '/favourites'.\n"
                                                            "In the meantime, feel free to tell me anything else that's troubling you.");
                flag = 1;
                return;
            } else if (check_no(message->text)) {
                bot.getApi().sendMessage(message->chat->id,
                                         "Alrighty then. Feel free to let me know of any more of your worries.");
                flag = 1;
                return;
            } else
                bot.getApi().sendMessage(message->chat->id,"Well that's confusing... :0 \nHelp me out here and key in a simple 'yes' or 'no' please.");
        }
    });

    //Capturing unexpected events
    signal(SIGINT, [](int s) {
        printf("SIGINT got\n");
        exit(0);
    });

    // Receiving user inputs via long poll
    try {
        printf("Bot username: %s\n", bot.getApi().getMe()->username. c_str());
        bot.getApi().deleteWebhook();
        TgLongPoll LongPoll (bot);
        int poll_on=1;
        while (poll_on) {
            printf("Long poll started\n");
            LongPoll.start();
            // Command to end polling
            bot.getEvents().onCommand("end", [&poll_on](const Message::Ptr& message){ poll_on = 0; });
        }
    }

    catch (exception& e) {
        printf("error: %s\n", e.what()); }
    return 0;
}

QuoteData.hpp

#ifndef CSV_TESTING_QUOTEDATA_HPP
#define CSV_TESTING_QUOTEDATA_HPP


struct QuoteTopic{
    explicit QuoteTopic (std::string const topic);
    ~QuoteTopic() = default;
    std::string generate_quote();
    std::vector<std::string> quotelist;
    int match_keywords (std::string const& sentence);
    std::string const Topic;
private:
    std::vector<std::string> generate_quotelist(std::string const topic);
};

#endif

QuoteData.cpp

#include <string>
#include <iostream>
#include <random>
#include <fstream>
#include <vector>
#include <utility> // std::pair
#include <stdexcept> // std::runtime_error
#include <sstream> // std::stringstream
#include "QuoteData.hpp"
#include "Functions.hpp"

//QuoteTopic Functions
QuoteTopic:: QuoteTopic(std::string const topic): Topic(topic){
    quotelist = generate_quotelist(topic);
}

std::string QuoteTopic:: generate_quote() {
    std::random_device rd;
    std::mt19937 gen(rd());
    std::uniform_int_distribution<> distr(0, quotelist.size()-1);
    return quotelist[distr(gen)];
}

std::vector<std::string> QuoteTopic::generate_quotelist(std::string const topic) {
    std::ifstream QuotesFile("quote_database.txt", std::ios::in | std::ios::binary);

    if (!QuotesFile.is_open())
        throw std::runtime_error("Could not open file");

    std::string line, word;

    if (QuotesFile.good()) {
        while (getline(QuotesFile, line, '\r')) {
            std::istringstream s(line);
            getline(s, word, '/');
            if (word == topic) {
                while (getline(s, word, '/')) {
                    quotelist.push_back(word);
                }
                break;
            }
        }
    }
    return quotelist;
}

int QuoteTopic::match_keywords(std::string const& sentence){
    //storing related keywords into a temporary vector
    std::ifstream KeywordsFile("topic_keywords.csv", std::ios::in | std::ios::binary);
    if (!KeywordsFile.is_open())
        throw std::runtime_error("Could not open file");
    std::string line, key;
    std::vector<std::string> keywords;
    if (KeywordsFile.good()) {
        while (getline(KeywordsFile, line, '\r')) {
            std::istringstream s(line);
            getline(s, key, ',');
            if (key == Topic) {
                while (getline(s, key, ',')) {
                    keywords.push_back(key);
                }
                break;
            }
        }
    }

    //counting the number of matched keywords
    int count = 0;
    std::string word;
    std::stringstream ss(sentence);
    while(ss>>word) {
        //if present, removes commas from strings
        word.erase(std::remove(word.begin(), word.end(), ','), word.end());
        for (const auto& e : keywords) {
            if (e == word)
                count++;
        }
    }
    return count;
}

Functions.hpp (functions not in QuoteTopic struct)

#ifndef INSPOBOT_FUNCTIONS_HPP
#define INSPOBOT_FUNCTIONS_HPP

std::vector<std::shared_ptr<QuoteTopic>> store_QuoteTopics();
int which_topic(std::string const& sentence, std::vector<std::shared_ptr<QuoteTopic>> AllTopics);

bool check_yes(std::string const& word);
bool check_no(std::string const& word);

bool compare_char (char const& a, char const& b);
bool compare_str (std::string const& a, std::string const& b);
bool operator==(std::string const& a, std::string const& b);
bool operator!=(std::string const& a, std::string const& b);


#endif

Functions.cpp

#include <string>
#include <iostream>
#include <random>
#include <fstream>
#include <vector>
#include <utility> // std::pair
#include <stdexcept> // std::runtime_error
#include <sstream> // std::stringstream
#include <boost/algorithm/string.hpp>
#include "QuoteData.hpp"
#include "Functions.hpp"

std::vector<std::shared_ptr<QuoteTopic>> store_QuoteTopics(){
    std::vector<std::shared_ptr<QuoteTopic>> AllTopics;
    std::ifstream QuoteFile("quote_database.txt", std::ios::in | std::ios::binary);

    if (!QuoteFile.is_open())
        throw std::runtime_error("Could not open file");
    std::string line, word;

    if (QuoteFile.good()) {
        while (getline(QuoteFile, line, '\r')) {
            std::istringstream s(line);
            getline(s, word, '/');
            AllTopics.push_back(std::shared_ptr<QuoteTopic>(new QuoteTopic(word)));
        }
    }
    return AllTopics;
}

int which_topic(std::string const& sentence, std::vector<std::shared_ptr<QuoteTopic>> AllTopics){
    int index = -1;
    int NumKeywords = 0;
    for(int n=0;n<AllTopics.size();n++){
        if (AllTopics[n]->match_keywords(sentence)>NumKeywords) {
            index = n;
            NumKeywords = AllTopics[index]->match_keywords(sentence);
        }
        else if ((AllTopics[n]->match_keywords(sentence)==NumKeywords)&&(NumKeywords!=0)){
            std::random_device rd;
            std::mt19937 gen(rd());
            std::uniform_int_distribution<> distr(1,2);
            index = (distr(gen) == 1)? index : n;
            NumKeywords = AllTopics[index]->match_keywords(sentence);
        }
    }
    return index;
}



//General Use Functions
bool check_yes(std::string const& word){
    std::string yes_words = "yep yes yeap sure of course indeed affirmative absolutely yup yah yeh yeet";
    if (yes_words.find(word) != std::string::npos)
        return true;
    else{
        std::string lower_word = boost::to_lower_copy(word);
        std::cout << lower_word << std::endl;
        return yes_words.find(lower_word) != std::string::npos;
    }
}

bool check_no(std::string const& word){
    std::string no_words = "no nope negative not at all nah no way naw not really absolutely not of course not";
    if (no_words.find(word) != std::string::npos)
        return true;
    else{
        std::string lower_word = boost::to_lower_copy(word);
        std::cout << lower_word << std::endl;
        return (no_words.find(lower_word) != std::string::npos);
    }
}

bool compare_char (char const& a, char const& b){
    return (a == b) || (std::toupper(a) == std::toupper(b));
}

bool compare_str (std::string const& a, std::string const& b){
    return ((a.size() == b.size())&&(std::equal(a.begin(), a.end(),b.begin(), &compare_char)));
}

bool operator==(std::string const& a, std::string const& b) {
    return compare_str(a, b);
}

bool operator !=(std::string const& a, std::string const& b) {
    return !(compare_str(a, b));
}

```
\$\endgroup\$
2
6
\$\begingroup\$

You are right about using namespace. The purpose of a namespace is to allow types and functions to have the same name in two or more areas of code. Think about the class configuration, its quite a common name for something in code so if you and I are both writing a module for an all singing all dancing app and we both have a configuration class we are screwed when we come to integrate it, if we are lucky it won't compile. So this is why you have namespaces, CodeGorilla::configuration and JessLim::configuration, don't have a problem being integrated and the project succeeds and we become millionaires. Then someone decided to stick a using namespace Bob; statement at the top of the code and everything turned ugly, because no one knows who's configuration was who's.

I think if you are going to use namespaces then there is almost never a reason to use a using namespace statement. There are always exceptions to any rule, but std is never one of them :)

I am sorry to say this so bluntly but IN MY OPINION your code is ugly. Its one monolithic function, it takes 8 scrolls to get to the bottom of it and sorry but my finger doesn't need that much exercise.

What can we do about it? Firstly you have these large strings dotted through the code. If you extracted each of those out into a constant that would make the code look better (assuming you named the constants nicely). Also that bit of refactoring allows you to make this multilingual.

Functions should not be ~200 lines I normally stick to less than a screen full which is about 40 lines. To be honest most of the functions I have written in the last few years and been 10 lines or less. You have to balance the increased readability from having short functions with the decreased readability from having to jump between functions. There is no one size fits all answer.

Lambda functions are great, but sometime they get to big and should be refactored into a separate function. bot.getEvents().onCallbackQuery() and bot.getEvents().onAnyMessage() could both do with being separated out.

Enums are your friend because they document the code without you having to write a comment. flag==1 and flag==2 are not very self explanatory, but flag == eMainChat and flag == eFeedbackChat are much more instantly recognisable.

Braces or { and } are really good at making code readable. Everyone has there own preference and I'm old school. You have written C++ and then formatted it as JavaScript, personally I go with an open and closing brace should almost always be on their own line, it just makes the code a little neater. Also always use braces, even on a single line. Again its just consistent and nice. When the code always looks the same then you don't read the code you just see it, I'm sorry that it's a bit of an airy fairy statement, but I can't think of a better way to describe it. You don't want to waste you time reading code you want to find the bug, fix it and move on.

You need to make sure all your variables are initialised. Compilers do this in debug builds but not normally in release builds. Classes are normally initialised in the constructor so std::string Return_Quote; is fine, but int TopicIndex; is not fine.

You also need to ensure that any memory you allocate is deallocated.
InlineKeyboardButton::Ptr KeyboardButton(new InlineKeyboardButton); is one occurrence but there are more.

Try and be consistent with your variable and function naming convention. Some variables are in lower case, some are capitalised, etc. And never, ever get lazy and use a single letter as a variable (n I am looking at you), It just means you have to add an extra comment where you could just have said topicIndex.

Oh and like the others said if you put all the information in the question people are more likely to answer it. I haven't followed your link, god knows what is there it could be a virus it could be wonderful code. Like I said my finger doesn't need more exercise and that extra click could just push it over the edge :)

\$\endgroup\$
1
  • \$\begingroup\$ there is almost never a reason to use a using namespace statement. But like you said there is always the exception. The place to use them is for your own code in your own compilation units. They make writing your code easier. But you should have a using namespace for a third party header. For third-party code if it is too lang winded use a namespace alias: namespace BMP = boost::MetaProgramming::Library; Now prefix all the code from that namespace with BMP. \$\endgroup\$ – Martin York Jun 17 '20 at 0:27

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.