9
\$\begingroup\$

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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ First of all: Ditch this using namespace std; please! \$\endgroup\$ – πάντα ῥεῖ Oct 10 '19 at 22:42
  • \$\begingroup\$ Does not a lot of the code rely on that though, or would i do like std::cout, etc for everything, im new to c++ so any tips help \$\endgroup\$ – SagittariusZodiac Oct 10 '19 at 22:44
  • \$\begingroup\$ Use explicit namespace scopes when needed, either like using std::cout; etc., or just prepend it when used like std::cout. See here: stackoverflow.com/questions/1452721/… \$\endgroup\$ – πάντα ῥεῖ Oct 10 '19 at 22:46
  • \$\begingroup\$ What version of C++ are you using? C++89? C++11 (or newer)? (And do you want to learn about the new stuff?) \$\endgroup\$ – hoffmale Oct 10 '19 at 22:52
  • 1
    \$\begingroup\$ I believe i am using c++17, using the 2019 community version of visual studio, and yes I want to learn a lot about new c++ stuff, just started learning c++ today \$\endgroup\$ – SagittariusZodiac Oct 10 '19 at 22:54
14
\$\begingroup\$

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?

| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ The code might look a little overwhelming for beginners at first. But this is a really nice display of good practices! \$\endgroup\$ – sbecker Oct 11 '19 at 6:57
  • \$\begingroup\$ @sbecker: Thanks! I tried to keep it very simple (and beginner-friendly), i.e. no templates, no noexcept, no constexpr, and so on. I also wanted to show some of the more modern c++ features in an easy-to-grasp way, as the code in question looked like it time-traveled around 30 years into the future. \$\endgroup\$ – hoffmale Oct 11 '19 at 16:03
  • \$\begingroup\$ Your code reading the input_choices runs into UB if the users enters a value outside the range. Please do range checking there! Note that the OPs code handled the invalid value by simply falling through the loop as ints were used. Here it works in practice but it is still UB. And a missing const on the range-based-for. \$\endgroup\$ – Flamefire Oct 14 '19 at 6:40
  • \$\begingroup\$ @Flamefire: Please clarify where I run into UB. Casting a value to an enum where the value is of the same type as the enums underlying type is well defined. As for error handling, I included a default case with a nice error message. \$\endgroup\$ – hoffmale Oct 14 '19 at 8:14
  • \$\begingroup\$ You are correct. I haven't seen you use a fixed type for the enum, so it is all good. Then just 2 nits: The const in the loop and enter_game can be static or a free function. \$\endgroup\$ – Flamefire Oct 14 '19 at 8:36

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.