Favorite Game List utilizing vectors and iterators

Question

So in learning c++ I have to make a little program that makes a game list that can add games to the list, remove games, and display the list, and it has to use vectors and iterators, so I made the program and need feedback on how to improve it and make it cleaner/better

// Favourite Game list
// Program using vectors and iterators

#include<iostream>
#include<string>
#include<algorithm>
#include<vector>
#include<ctime>
#include<cctype>
#include<cstdlib>


using namespace std;

int main()
{
    bool quit = false;
    string game_name;
    vector<string> gameList;
    vector<string>::iterator myIterator;
    vector<string>::const_iterator iter;
    int choice;
    while (!quit)
    {
        cout << "1: Add a Game to List" << endl;
        cout << "2: Remove a Game from the List" << endl;
        cout << "3: List all the Games on the List" << endl;
        cout << "4: Exit the Program" << endl;
        cin >> choice;

        switch (choice)
        {
        case 1:
            cout << "Enter the Game Name to add" << endl;
            cin >> game_name;
            gameList.push_back(game_name);
            break;
        case 2:
            for (iter = gameList.begin(); iter != gameList.end(); ++iter)
                cout << *iter << endl;
            cout << "Here is the list of games enter the one to remove" << endl;
            cin >> game_name;
            iter = find(gameList.begin(), gameList.end(), game_name);
            cout << *iter << "This is the game we will remove" << endl;
            gameList.erase(iter);

            break;


        case 3:
            cout << "Your Games" << endl;
            for (iter = gameList.begin(); iter != gameList.end(); ++iter)
                cout << *iter << endl;
            break;

        case 4:
            quit = true;
            cout << "You chose quit! The Program Will Close Now" << endl;

        }

    }






    return 0;
}

Answer 1

Unused headers

As far as I can tell, nothing from the following headers is actively being used, so they shouldn't be included.

• <ctime>
• <cctype>
• <cstdlib>

using namespace std

While it's probably not quite as bad using it inside a .cpp file instead of a header, it's generally seen as a code smell.

Variable usage

There are some variables that are declared at the begin of main, but only used further down (if at all, myIterator appears to not be used at all). This makes it harder to keep track of variables.

Magic numbers/literals

There are a lot of numbers/string literals, whose intention is not obvious in all cases. Examples include values 1 to 4 for input choices, or strings like "Your Games".

One way of dealing with these so called magic numbers is to put them into adequately named enums, constants or variables.

Container choice

There doesn't seem to be any dependency on actually keeping the order of the games intact. In addition, a small oversight (it is possible to add the same game twice) leads me to suggest that maybe a different container would be a better fit: std::unordered_set.

std::unordered_set allows us to insert and remove items with amortized \$\mathcal{O}(1)\$ runtime complexity, as well as to ignore duplicates.

Iterators or ranged for loops?

Generally, I prefer ranged for loops over "manual" iterator loops if the code isn't requiring some specific iterator behavior. Since this isn't the case here, I would suggest using ranged for loops for ease of reading.

Prefer smaller functions

Generally, it is easier to work with smaller functions: There is less mental overhead due to shorter code, fewer variables and narrower scope. Additionally, it allows for code reuse, which might reduce code duplication.

While the given code is small enough that these concerns might not matter right now, it will become a lot more obvious once additional logic like formatting, input handling or similar gets added.

Error checking

Neither user inputs nor results of calls to std::find are being checked for invalid values. This means I can easily cause errors inside the program (or crash it) by just entering weird stuff, or trying to remove non-existing entries.

Summary

Using all these suggestions, a cleaned up version could look like this:

#include <iostream>
#include <string>
#include <unordered_set>

enum class input_choices : int {
    add = 1,
    remove,
    list,
    exit
};

std::ostream& operator<<(std::ostream& output, input_choices choice) {
    output << static_cast<int>(choice);
    return output;
}

std::istream& operator>>(std::istream& input, input_choices& choice) {
    int temp = 0;
    input >> temp;
    choice = static_cast<input_choices>(temp);
    
    return input;
}

namespace messages {
    static const auto menu_header = "Menu";
    static const auto option_add = "Add a game to the list";
    static const auto option_remove = "Remove a game from the list";
    static const auto option_list = "Show all games on the list";
    static const auto option_exit = "Exit the program";
    static const auto enter_menu_choice = "Your choice: ";

    static const auto enter_game_to_add = "Enter the Game Name to add";
    static const auto enter_game_to_remove = "Here is the list of games enter the one to remove";
    static const auto list_header = "Your games";
    static const auto quitting = "Exiting program...";
    static const auto invalid_choice = "Unknown menu option";
}

class game_list_menu {
    std::unordered_set<std::string> games;

public:
    void add_game() {
        const auto game = enter_game(messages::enter_game_to_add);
        games.insert(game);
    }

    void remove_game() {
        print_games();

        const auto game = enter_game(messages::enter_game_to_remove);
        games.erase(game);
    }
    
    std::string enter_game(std::string_view prompt) const {
        std::cout << prompt << ": ";
        
        auto game = std::string{};
        std::cin >> game;
        
        return game;
    }


    void print_games() const {
        std::cout << "\n" << messages::list_header << ":\n\n";
        
        for(auto& game : games) {
            std::cout << "\t" << game << "\n";
        }
    }

    void print_menu() const {
        std::cout << "\n" << messages::menu_header << "\n\n";
        
        print_choice(input_choices::add, messages::option_add);
        print_choice(input_choices::remove, messages::option_remove);
        print_choice(input_choices::list, messages::option_list);
        print_choice(input_choices::exit, messages::option_exit);
        
        std::cout << "\n" << messages::enter_menu_choice;
    }

    void print_choice(input_choices choice, std::string_view description) const {
        std::cout << "\t" << choice << ": " << description << "\n";
    }

    void main_loop() {
        auto choice = input_choices::exit;

        do {
            print_menu();
            
            std::cin >> choice;

            switch(choice) {
            case input_choices::add:
                add_game();
                break;
            case input_choices::remove:
                remove_game();
                break;
            case input_choices::list:
                print_games();
                break;
            case input_choices::exit:
                std::cout << messages::quitting << "\n";
                break;
            default:
                std::cout << messages::invalid_choice << "\n";
                break;
            }
        } while(choice != input_choices::exit);
    }
};

int main() {
    auto menu = game_list_menu{};
    menu.main_loop();
}

Can you see how you can easily tell what each function does or what the meaning of each number/literal is?