0
\$\begingroup\$

The below code segments compose a simple console game where the user is prompted to enter a randomly generated letter displayed in the console. It also saves and loads user data through a .txt file.

It's a good bit of code, so I'm looking for any sort of constructive criticism on the code. Feedback on how to achieve the utmost optimization and how to improve the design of the program overall would be greatly appreciated. Even a skim of the code with a brief analysis would be great. Feedback on any aspect of the code would be great.

main.cpp:

#include "PLAYER_DATABASE_H.h"

const std::string player_data_file = "player_data.txt";
const std::string quit_prompt_msg = "DONE";

enum menu_options { LOGIN=1, REGISTER=2, EXIT=3 };

std::string menu_options_to_string(menu_options option);

menu_options main_menu(const Player_database &database, std::ostream &output, std::istream &input);

void prompt_username_and_password(std::string &username, std::string &password, std::ostream &output, std::istream &input);

Player* prompt_login(Player_database &database, std::ostream &output, std::istream &input, const std::string &quit_msg);

Player* prompt_register_account(Player_database &database, std::ostream &output, std::istream &input, const std::string &quit_msg);

bool quit_prompt(std::istream &input, std::ostream &output, const std::string &msg, const std::string &quit_msg);

int main() {
    
    std::cout << std::numeric_limits<points_t>::max() << "\n\n";
    Player *player;
    Player_database database(player_data_file);
    
    menu_options choice = main_menu(database, std::cout, std::cin);
    
    switch (choice) {
        case LOGIN: {
            player = prompt_login(database, std::cout, std::cin, quit_prompt_msg);
            break;
        }
        case REGISTER: {
            player = prompt_register_account(database, std::cout, std::cin, quit_prompt_msg);
            break;
        }
        default: {
            player = nullptr;
        }
    }

    if (player != nullptr) {
        Game game(player);
        
        while (true) {
            game.play_round(std::cout, std::cin);
            
            if (game.point_limit_reached()) {
                std::cout << "\nYou've reached the maximum number of points! If you score another point, your points will be reset to 0.\n\n\n";
            }
            
            char answer;
            std::cout << "Play again? (y/n): ";
            std::cin >> answer;
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
            std::cout << '\n';
            
            if (answer == 'n' || answer == 'N') break;
        }
    }
    
    std::cout << "Exited.";
}

std::string menu_options_to_string(menu_options option) {
    switch (option) {
        case LOGIN: return "LOGIN";
        case REGISTER: return "REGISTER";
        default: return "EXIT";
    }
}

menu_options main_menu(const Player_database &database, std::ostream &output, std::istream &input) {
    unsigned short option = 0;
    
    output << "Type the corresponding number of the action to be executed\n";
    for (int elem = LOGIN; elem != EXIT; ++elem) {
        output << elem << ". " << menu_options_to_string(static_cast<menu_options>(elem)) << '\n';
    }
    output << "\nChoice: ";
    
    if (!(input >> option) || option > REGISTER) return EXIT;
    
    input.ignore(1, '\n'); // get rid of newline character
    
    return static_cast<menu_options>(option);
}

void prompt_username_and_password(std::string &username, std::string &password, std::ostream &output, std::istream &input) {
    output << "Username: ";
    std::getline(input, username);
    
    output << "Password: ";
    std::getline(input, password);
}

Player* prompt_login(Player_database &database, std::ostream &output, std::istream &input, const std::string &quit_msg) {
    std::string username, password;
    bool valid_entry;
    
    do {
        prompt_username_and_password(username, password, output, input);
        
        valid_entry = database.does_account_exist(username, password);
        
        if (!valid_entry) {
            if (quit_prompt(input, output, "Invalid attempt. Type " + quit_msg + " to exit prompt, anything else to continue", quit_msg)) return nullptr;
        }
    } while (!valid_entry);
    
    return database.login(username, password);
}

Player* prompt_register_account(Player_database &database, std::ostream &output, std::istream &input, const std::string &quit_msg) {
    std::string username, password;
    bool valid_entry;
    
    do {
        prompt_username_and_password(username, password, output, input);
        
        valid_entry = !database.does_username_exist(username);
        
        if (!valid_entry) {
            if (quit_prompt(input, output, "An account with that username already exists. Type " + quit_msg + " to exit prompt, anything else to continue", quit_msg)) return nullptr;
        }
    } while(!valid_entry);
    
    return database.register_account(username, password);
}

bool quit_prompt(std::istream &input, std::ostream &output, const std::string &msg, const std::string &quit_msg) {
    std::string response;
    
    output << '\n' << msg << ": ";
    std::getline(input, response);
    output << '\n';
    
    return (response == quit_msg);
}

GAME_H:

#ifndef GAME_H
#define GAME_H

#include <chrono>
#include <limits>
#include "PLAYER_H.h"
#include <random>

class Game {
    public:
        explicit Game(Player *_player) : player(_player) {}
        void play_round(std::ostream &output, std::istream &input);
        bool point_limit_reached() const;
    private:
        Player *player;
};

#endif

game.cpp:


std::random_device seed;
std::uniform_int_distribution<int> uid(65, 90); // where 65 in ASCII = A and 90 = Z

void Game::play_round(std::ostream &output, std::istream &input) {
    char correct_answer = uid(seed);
    char player_answer;
    
    output << correct_answer << '\n';
    output << "Type the character above: ";
    input >> player_answer;
    input.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    
    if (player_answer == correct_answer || player_answer == static_cast<char>(correct_answer+32)) {
        player->increment_points(); // 32 is lowercase correspondent of answer in ASCII
        output << "+1 point\n";
    } else {
        player->decrement_points();
        output << "-1 point\n";
    }
    
    output << "Your points: " << player->get_points() << "\n\n";
}

bool Game::point_limit_reached() const {
    return (player->get_points() == std::numeric_limits<points_t>::max());
}

PLAYER_H:

#define PLAYER_H

#include <iostream>
#include <string>

using points_t = unsigned long long;

class Player {
    public:
        explicit Player(std::istream &input);
        Player(std::string _username, std::string _password, points_t _points=0) : username(_username), password(_password), points(_points) {}
        void increment_points();
        void decrement_points();
        points_t get_points() const;
        bool is_my_password(const std::string &_password) const;
        bool operator<(const Player &other_player) const;
        bool operator>(const Player &other_player) const;
        bool operator==(const Player &other_player) const;
        friend std::ostream& operator<<(std::ostream &output, const Player &player);
    private:
        std::string username, password;
        points_t points;
};
std::ostream& operator<<(std::ostream &output, const Player &player);

#endif

player.cpp:


Player::Player(std::istream &input) {
    input >> username >> password >> points;
}

void Player::increment_points() {
    ++points;
}

void Player::decrement_points() {
    if (!points--) points = 0;
}

points_t Player::get_points() const {
    return points;
}

bool Player::is_my_password(const std::string &_password) const {
    return (password == _password);
}

bool Player::operator<(const Player &other_player) const {
    return (username < other_player.username);
}

bool Player::operator>(const Player &other_player) const {
    return (username > other_player.username);
}

bool Player::operator==(const Player &other_player) const {
    return (username == other_player.username);
}

std::ostream& operator<<(std::ostream &output, const Player &player) {
    output << player.username << ' ' << player.password << ' ' << player.points;
    
    return output;
}

PLAYER_DATABASE_H:

#define PLAYER_DATABASE_H

#include <algorithm>
#include <fstream>
#include "PLAYER_H.h"
#include <vector>

class Player_database {
    public:
        explicit Player_database(std::string _database_file);
        ~Player_database();
        Player* find_player(const std::string &username, const std::string &password);
        bool does_username_exist(const std::string &username);
        bool does_account_exist(const std::string &username, const std::string &password);
        Player* register_account(const std::string &username, const std::string &password);
        Player* login(const std::string &username, const std::string &password);
    private:
        bool does_file_exist() const;
        void read_file_data();
        void write_file_data() const;
        using player_vector = std::vector<Player>;
        player_vector players;
        const std::string database_file;
};

#endif

player_database.cpp:


Player_database::Player_database(std::string _database_file) : database_file(_database_file) {
    if (does_file_exist()) read_file_data();
}

Player_database::~Player_database() {
    write_file_data();
}

Player* Player_database::find_player(const std::string &username, const std::string &password) {
    auto player_iter = std::find(players.begin(), players.end(), Player(username, password));
    if (player_iter == players.end()) return nullptr;
    else return &*player_iter;
}

bool Player_database::does_username_exist(const std::string &username) {
    return std::binary_search(players.begin(), players.end(), Player(username, ""));
}

bool Player_database::does_account_exist(const std::string &username, const std::string &password) {
    Player *player = find_player(username, password);
    
    if (!player || !player->is_my_password(password)) return false;
    return true;
}

Player* Player_database::register_account(const std::string &username, const std::string &password) {
    Player player(username, password);
    auto lb = std::lower_bound(players.begin(), players.end(), player);
    players.insert(lb, player);
    
    return &(players[lb-players.begin()]);
}

Player* Player_database::login(const std::string &username, const std::string &password) {
    return find_player(username, password);
}

bool Player_database::does_file_exist() const {
    std::ifstream checker(database_file);
    return checker.good();
}

void Player_database::read_file_data() {
    std::ifstream file_in(database_file);
    
    while (file_in.peek() != -1) { // is empty file?
        players.push_back(Player(file_in));
        file_in.ignore(1, '\n');
    }
    
    file_in.close();
}

void Player_database::write_file_data() const {
    std::ofstream file_out(database_file, std::ofstream::trunc);
    
    for (auto &player : players) {
        file_out << player << '\n';
    }
    
    file_out.close();
}
```
\$\endgroup\$
4
  • \$\begingroup\$ I thought decrementing a signed int type when it's 0 would work like a clock and cycle to the maximum value it could equal? \$\endgroup\$ Jun 2 at 20:20
  • \$\begingroup\$ @matmartelli No, but it will behave that way for unsigned types. For signed, the optimizer will assume that this never happens and not generate the code you expected. Here, the compiler will see that the entire expression exhibits UB when the value was 0, so it will assume it is never called when the value is 0, and throw away the if branch in that case. \$\endgroup\$
    – JDługosz
    Jun 3 at 14:29
  • \$\begingroup\$ I'm not sure if this is just going completely over my head or I'm in a different thought process. But a signed type, decremented from 0, would just equal -1? \$\endgroup\$ Jun 3 at 21:12
  • \$\begingroup\$ you're right, a signed type will decrement 0 to -1. I saw you mention "like a clock and cycle to the maximum value" and that would be for the smallest (negative) value it can hold, not zero. My remarks concern that signed types do not "wrap around". My first comment was wrong; I was also mixing up 0 and smallest-it-can-hold. \$\endgroup\$
    – JDługosz
    Jun 4 at 14:55
2
\$\begingroup\$

Use constexpr and also use string_view to avoid copying the contents:

constexpr std::string_view player_data_file = "player_data.txt";

Use string_view for parameters instead of const string&.

Don't compare directly against nullptr. Use the truth tests that are part of the pointer (or smart pointer!) types: (player != nullptr) should be if(!player)

const Player_database &database

In C++, it is normal to put the & ir * qualifier with the type not with the name being declared. Write: const Player_database& database).

more (added later)

Your main function is meandering. Functions should be cohesive and do one thing. In general, separate your program into input, "the real work", and output.

int main() {
    
    std::cout << std::numeric_limits<points_t>::max() << "\n\n";

I don't know why you are printing the maximum number here.

    Player *player;

This is declared without being initialized. This is one clue that you have a section of code that really should be broken out into a separate function. E.g.: Player* player = load_player();

All the way from here----

    Player_database database(player_data_file);
    
    menu_options choice = main_menu(database, std::cout, std::cin);
    
    switch (choice) {
        case LOGIN: {
            player = prompt_login(database, std::cout, std::cin, quit_prompt_msg);
            break;
        }
        case REGISTER: {
            player = prompt_register_account(database, std::cout, std::cin, quit_prompt_msg);
            break;
        }
        default: {
            player = nullptr;
        }
    }

----- to here, can be the contents of load_player(). Being a separate function, that code can just return when it has the player, from each spot. Notice also that database is only used by this passage of code, another sign that this clump of code forms a cohesive subroutine on its own.

    if (player != nullptr) {

Here you have the entire rest of the function inside the body of an if, which includes more variables and an even deeper loop. It would be clearer and simpler to just get out if the test fails. Then the rest of the program is on the main level of the function.

What follows can itself be another function or even two. The user interaction should be in a separate function. Again, look at cohesiveness: is answer needed by the rest of the code? No, only for these few lines. Taken as a group, what is needed by these lines? Nothing; it does not care about the player or the game objects. What is produced? Just a bool result.

        Game game(player);
        
        while (true) {
            game.play_round(std::cout, std::cin);
            
            if (game.point_limit_reached()) {
                std::cout << "\nYou've reached the maximum number of points! If you score another point, your points will be reset to 0.\n\n\n";
            }
            
            char answer;
            std::cout << "Play again? (y/n): ";
            std::cin >> answer;
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
            std::cout << '\n';
            
            if (answer == 'n' || answer == 'N') break;
        }
    }
    
    std::cout << "Exited.";
}

others

std::uniform_int_distribution<int> uid(65, 90); // where 65 in ASCII = A and 90 = Z
Why not just use uid('A','Z') directly? Chars implicitly convert to int, you know. Or if you must, int{'A'} etc. to be explicit.


if (player_answer == correct_answer || player_answer == static_cast<char>(correct_answer+32)) {
How about if (correct_answer == toupper(player_answer) { ? You don't need to reveal exactly how upper and lower case are related, the meaning is clearly read, and it's better to apply the transformation to the typed input rather than the "correct" value.

\$\endgroup\$
6
  • \$\begingroup\$ I excluded 'const' for 'Player_database &database' in the prompt_login and prompt_register_account functions because in the function it access a modifier function member of database. Is that what you were addressing? \$\endgroup\$ Jun 1 at 21:01
  • \$\begingroup\$ I don't think so, since I didn't mention anything about const. For database I'm referring to the whitespace. \$\endgroup\$
    – JDługosz
    Jun 2 at 13:40
  • \$\begingroup\$ Ah okay, I see, my bad. Didn't know what ir was, it's meant to be or duh. \$\endgroup\$ Jun 2 at 17:45
  • \$\begingroup\$ Can I ask, were these comments from a quick skim of the code? Or were these the only issues you noticed from thoroughly reading the entire code? \$\endgroup\$ Jun 2 at 17:47
  • \$\begingroup\$ @mastmartelli they are from a quick look. These are things that apply to an individual declaration or expression, easily spotted in isolation. \$\endgroup\$
    – JDługosz
    Jun 2 at 18:35

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.