Custom card game

Question

I had a task to create a program to simulate a simple card game. Given an input file with number of players, the players and finally a list of cards played, I had to change the game state accordingly (I believe card-action relations are clear enough from the code, so I won't go into detail here).

The task was not intended to be done with OOP, and required me to have certain functions. I implimented those functions as methods. Some of them might seem obsolete (i.e. getWinner()), but I cannot remove them.

I'd like you to review my code and give feedback (well, duh :P ). I'd be very happy if you adressed these points:

• Did I use proper OOP?
• Did I handle exceptions properly? If not, what did I do wrong?
• Does my use of std::move() actually change anything and help in any way?
• In my exception definitions, there is a bunch of duplicate code, can I somehow fix that?

My code:

game.h:

#ifndef GAME_H
#define GAME_H

#include <vector>
#include <stack>
#include <string>
#include <fstream>
#include <algorithm>
#include <exception>
#include <stdexcept>

// Exception structs
class input_file_error : public std::exception
{
public:
    input_file_error(const char* message) : msg_(message) {}
    input_file_error(const std::string& message) : msg_(message) {}
    const char* what() const throw()
    {
        return msg_.c_str();
    }

protected:
    std::string msg_;
};

class game_runtime_error: public std::exception
{
public:
    game_runtime_error(const char* message) : msg_(message) {}
    game_runtime_error(const std::string& message) : msg_(message) {}
    const char* what() const throw()
    {
        return msg_.c_str();
    }

protected:
    std::string msg_;
};

// Only used together with game
struct Player
{
    std::string name;
    std::string surname;
    int score;
    friend std::ostream& operator<<(std::ostream& out, const Player& player);
};

class Game
{
private:
    // Current players
    std::vector<Player> m_players;

    // Current players that are out
    // Only the top player shall be accessed, the order
    // Shall not be changed
    std::stack<Player, std::vector<Player>> m_outPlayers;

    // Cards in game, game ends when no more cards are left
    std::string m_cardPlays;

    // Card symbols
    static const char cardAdd = '+';
    static const char cardRemove = '-';
    static const char cardNameSort = 'V';
    static const char cardSurnameSort = 'P';
    static const char cardScoreSort = '~';

    // Only used during initialisation
    bool isValidCard(const char card) const throw();
    Player getWinner() const throw();
    void sortByName() throw();
    void sortBySurname() throw();
    void sortByAlternatingScore() throw();
    // We can assume that this never fails
    void removeLastPlayer() throw();
    // If this fails, it shall do nothing
    void bringBackLastPlayer() throw();
    // Updates scores, never fails
    void updateScores() throw();
    // Prints the winner to the given file
    void printResults(const std::string& fName) const throw();

public:
    // Takes a file from which it should initialize the game
    // Throws exceptions on invalid initialisation
    Game(const std::string& fName);
    // Execute the game, return the winner
    // Throws if m_cardPlays is empty at the start of the execution
    void play(const std::string& fName);
    void getDataFromFile(const std::string& fName);

    // For debugging
    friend std::ostream& operator<<(std::ostream& out, const Game& cGame);
};

#endif // GAME_H

game.cpp:

#include "game.h"
#include <sstream>
#include <utility>

// Temporary, for debugging
#include <iostream>

std::ostream& operator<<(std::ostream& out, const Player& player)
{
    out << player.name << ' ' << player.surname << ' '
    << player.score << '\n';
    return out;
}

void Game::getDataFromFile(const std::string& fName)
{
    // Clear the old data
    m_players.clear();
    while(!m_outPlayers.empty()) {
        m_outPlayers.pop();
    }

    std::ifstream in(fName);
    if(!in) {
        throw input_file_error("Unable to open the file.");
    }

    int numOfPlayers;
    in >> numOfPlayers; in.ignore();

    if(!in) {
        throw input_file_error("The number of players is missing.");
    }

    if(numOfPlayers < 4) {
        throw input_file_error("Not enough players to play the game");
    }
    if(numOfPlayers > 20) {
        throw input_file_error("Too many players to play the game.");
    }

    int pCount(0);
    while(pCount < numOfPlayers && in) {
        std::string line;
        std::getline(in, line);

        // Skip empty lines
        if(line.empty()) {
            continue;
        }
        ++pCount;

        std::stringstream ss(line);
        Player tPlayer;
        ss >> tPlayer.name >> tPlayer.surname;

        // If a card list is used as player input
        // Also if only name/surname of a player is given
        if(tPlayer.surname.empty()) {
            throw input_file_error("Given and actual number of players does not match.");
        }
        // Default score
        tPlayer.score = 100;
        m_players.push_back(std::move(tPlayer));
    }

    if(!in) {
        throw input_file_error("Given and actual number of players does not match, card play list is missing.");
    }

    // Skip empty lines
    while(std::getline(in, m_cardPlays)) {
        if(!m_cardPlays.empty()) {
            break;
        }
    }

    if(m_cardPlays.empty()) {
        throw input_file_error("Card play list missing.");
    }

    // Cards are given in the order they are played
    std::reverse(m_cardPlays.begin(), m_cardPlays.end());

    bool playListFine = true;
    for(size_t i = 0; i < m_cardPlays.size(); ++i) {
        if(!isValidCard(m_cardPlays.at(i))) {
            playListFine = false;
            break;
        }
    }

    // If invalid card list was given
    if(!playListFine) {
        throw input_file_error("Invalid card play list is given.");
    }
}

Game::Game(const std::string& fName)
{
    getDataFromFile(fName);
}

std::ostream& operator<<(std::ostream& out, const Game& cGame)
{
    for(auto it = cGame.m_players.begin(); it != cGame.m_players.end(); ++it) {
        out << *it;
    }
    out << cGame.m_cardPlays << '\n';
    return out;
}

bool Game::isValidCard(const char card) const throw()
{
    switch(card)
    {
    case cardAdd:
    case cardNameSort:
    case cardScoreSort:
    case cardSurnameSort:
    case cardRemove:
        return true;
    default:
        return false;
    }
}

// Returns the player with the highest score. If scores of 2 players are equal, return the one higher in the list
Player Game::getWinner() const throw()
{
    const Player* winner = &m_players.at(0);
    for(auto it = m_players.begin() + 1; it != m_players.end(); ++it) {
        if(it->score > winner ->score) {
            winner = &(*it);
        }
    }
    return *winner;
}

void Game::sortByName() throw()
{
    std::sort(m_players.begin(), m_players.end(), [](const Player& p1, const Player& p2) { return p1.name < p2.name; });
}

void Game::sortBySurname() throw()
{
    std::sort(m_players.begin(), m_players.end(), [](const Player& p1, const Player& p2) { return p1.surname < p2.surname; });
}

// Is there a nicer way to do this?
void Game::sortByAlternatingScore() throw()
{
    std::sort(m_players.begin(), m_players.end(), [](const Player& p1, const Player& p2) {return p1.score > p2.score; });

    auto maxIt = m_players.begin();
    auto minIt = m_players.end() - 1;
    std::vector<Player> tVec;
    tVec.reserve(m_players.size());
    while(maxIt <= minIt) {
        tVec.push_back(*maxIt++);
        if(maxIt < minIt) {
            tVec.push_back(*minIt--);
        }
    }
    m_players = tVec;
}

// Removes last player from the list, adds it to outPlayers list
void Game::removeLastPlayer() throw()
{
    m_outPlayers.push(std::move(m_players.back()));
    m_players.pop_back();
}


/** Unclear conditions, how should this player be positioned in the
    list? Need to ask **/ // For now ignore this
// Removes player from outPlayers list, adds it to player list
void Game::bringBackLastPlayer() throw()
{
    if(m_outPlayers.size() == 0) {
        return;
    }
    m_players.push_back(std::move(m_outPlayers.top()));
    m_outPlayers.pop();
}

void Game::updateScores() throw()
{
    int scoreMultiplier = m_players.size();
    for(size_t i = 0; i < scoreMultiplier; ++i) {
        m_players.at(i).score += (scoreMultiplier - i) * 10;
    }
}

void Game::play(const std::string& fName)
{
    if(m_cardPlays.empty()) {
        throw game_runtime_error("Unable to play, empty card play list.");
    }

    while(!m_cardPlays.empty()) {
        char cCard = m_cardPlays.back();
        m_cardPlays.pop_back();

        switch(cCard)
        {
        case cardAdd:
            bringBackLastPlayer();
            break;
        case cardNameSort:
            sortByName();
            break;
        case cardRemove:
            removeLastPlayer();
            break;
        case cardScoreSort:
            sortByAlternatingScore();
            break;
        case cardSurnameSort:
            sortBySurname();
            break;
        // No need to check for card validity, already did that in
        // the constructor
        }
        updateScores();

        // For debugging
        std::cout << *this << '\n';
    }

    printResults(fName);
}

void Game::printResults(const std::string& fName) const throw()
{
    std::ofstream out(fName);
    out << getWinner();
}

main.cpp:

#include <iostream>
#include "game.h"

int main()
{
    const std::string inDat = "Duomenys.txt";
    const std::string outWin = "Laimetojas.txt";
    try
    {
        Game game(inDat);
        std::cout << game << '\n';
        game.play(outWin);
        game.getDataFromFile(inDat);
        game.play(outWin);
    }
    catch(input_file_error& e)
    {
        std::cout << "Error while operating the input file: \n"
        << e.what() << '\n';
    }
    catch(game_runtime_error& e)
    {
        std::cout << "Game error: \n"
        << e.what() << '\n';
    }
    catch(...)
    {
        std::cout << "Unknown exception.";
    }
}

EDIT: I forgot to add the input file... Welp, I posted this at 3 am, just had to miss something... Here it is:

Duomenys.txt

6

Blezas Paskalis

Petras Klaustukas

Balys Sruoga

Sarpis Idomutis

Informikas Baltutis

Konstitucija Balsute

V-~P~++V

EDIT: As requested, I wrote a brief summary of the game rules.

A pile of cards is given (last line of the input file), they are played in the order they are given. The game ends when no more cards are left. Each turn, one card is played. What each card does:

• 'V' - the list of players is sorted by players' names, greater names first (Anna goes before Bob, etc.)
• 'P' - the list of players is sorted by players' surnames, greater surnames first (Jerkins before Zuzan etc.)
• '-' - the last player in the list is removed from the game (and added to the list of removed players)
• '+' - the last removed player goes back into the game (If Johny was removed, then Pete was removed, and then this card was played, Pete goes back into the game)
• '~' - the list of players is sorted in alternating order by score (dunno how to word this; First goes the player with max score, then the one with min score, then the one with max score from the rest, then the one with min score from the rest etc.)

After each turn (after one card is played) players' scores are updated according to this algorithm:

• First player in the list gets n * 10 score, where n is the number of players currently in-game;
• Second player in the list gets (n - 1) * 10 score;
• Third gets (n - 2) * 10 score and so on and so forth.

The winner is the player who has the biggest score. The number of players has to be between 3 and 20. We can assume that whenever a '-' card is played, there is at least one player left in the list (the number of '-' cards in the deck is lower than the number of players). The starting player list is as given in the file. If at the end of the game multiple people have the winning score, the one who is higher in the list wins.

I hope I didn't miss anything. Sorry if my english is bad, feel free to correct both my code and my language^^

Answer 1

I see some things that may help you improve your program.

Keep implementation details private

One of the cornerstones of OOP is the concept of information hiding. That is, the interface is public but the implementation is generally private. In this code, it's done pretty well but could be improved. In particular, the Player class could be entirely within the Game class since, as the comment notes, it's only used from within that class.

Don't use deprecated features

The throw() dynamic exception specification is going away for C++17 and has been deprecated for a while. The new mechanism is to use noexcept. However, with that said, see the next point.

Don't overspecify your interface

The use of throw() on so many functions is probably not a good idea for two reasons. First, it obligates your interface to never throw no matter how you might wish to re-implement the functions later. In almost every case, that's not really helpful because the compiler can often figure out for itself whether a function is noexcept or not. Second, it doesn't matter much for performance for many compilers. See this question for more views on this topic.

Use range-for to simplify your code

Since you're using C++11, you can use range-for to simplify your code. For example, the current code includes this:

std::ostream& operator<<(std::ostream& out, const Game& cGame)
{
    for(auto it = cGame.m_players.begin(); it != cGame.m_players.end(); ++it) {
        out << *it;
    }
    out << cGame.m_cardPlays << '\n';
    return out;
}

It can be simplified to this:

std::ostream& operator<<(std::ostream& out, const Game& cGame)
{
    for(const auto& player: cGame.m_players) 
    {
        out << player;
    }
    return out << cGame.m_cardPlays << '\n';
}

Be careful with signed versus unsigned

The code currently contains this function:

void Game::updateScores() throw()
{
    int scoreMultiplier = m_players.size();
    for(size_t i = 0; i < scoreMultiplier; ++i) {
        m_players.at(i).score += (scoreMultiplier - i) * 10;
    }
}

However, on some platforms, size_t is unsigned, so the loop is comparing an unsigned and a signed number. I'd avoid that issue and simplify the code like this:

void Game::updateScores() 
{
    auto scoreAdder = m_players.size() * 10;
    for(auto& player : m_players) {
        player += scoreAdder;
        scoreAdder -= 10;
    }
}

Note that this requires the implementation of Player::operator+= but it's quite simple:

Player& operator+=(int scoreAdder) { 
    score += scoreAdder;
    return *this;
}

Use standard algorithms

The getWinner() code can be greatly simplified by using std::max_element:

Game::Player Game::getWinner() const noexcept
{
    return *std::max_element(m_players.begin(), m_players.end(), [](const Player& p1, const Player& p2) { return p1.score < p2.score; });
}

Use standard exceptions

Instead of using the two exception classes defined in the current code, just use std::runtime_error which eliminates redundant code and simplifies. Alternatively derive your custom classes from std::runtime_error if you really need to distinguish between them.

class game_runtime_error: public std::runtime_error
{
public:
    game_runtime_error(const char *message) : std::runtime_error(message) {}
};

Let the compiler destroy objects

Within getDataFromFile we have this code:

// Clear the old data
m_players.clear();
while(!m_outPlayers.empty()) {
    m_outPlayers.pop();
}

However this almost exactly what we'd expect of a default constructor with no arguments. I'd formalize that by doing it this way instead:

{
    Game g;
    std::swap(*this, g);
}

The braces are there to tell the compiler that g is OK to be automatically deleted when that scope ends. If you don't want to have it possible to create an empty Game, make the empty constructor private:

Game() {};

Reconsider the interface design

Right now, the Game::getDataFromFile() routine takes a std::string but it might be better to have it take a std::istream& instead. That way, it could easily be adapted to accept the input even from things that are not files, such as from std::cin.

Define a constructor for Player

Right now, the Player object is instantiated with code in Game. It would be better OOP design to have a constructor for Player and delegate instead to that. Here's how I'd write it:

Player(std::string line) : score{100} {
    std::stringstream ss(line);
    ss >> name >> surname;
}

Use emplace_back where practical

The current code has this line:

m_players.push_back(std::move(tPlayer));

However, there's a std::vector member function which better expresses this. It's emplace_back:

m_players.emplace_back(tPlayer);

Avoid break where practical

Using break means that the control flow is something other than the usual and makes the code a little harder to read than if an alternative is easily available. For example, the code includes this:

// Skip empty lines
while(std::getline(in, m_cardPlays)) {
    if(!m_cardPlays.empty()) {
        break;
    }
}

The comment helps, but I think I'd be inclined to write the loop like this instead:

while(std::getline(in, m_cardPlays) && m_cardPlays.empty())
{}

Fail early if possible

The constructor for Game currently includees this:

bool playListFine = true;
for(size_t i = 0; i < m_cardPlays.size(); ++i) {
    if(!isValidCard(m_cardPlays.at(i))) {
        playListFine = false;
        break;
    }
}

// If invalid card list was given
if(!playListFine) {
    throw std::runtime_error("Invalid card play list is given.");
}

This could be much simplified since there isn't much point to continuing looking at cards once one invalid one is found:

for (const auto card : m_cardPlays) {
    if (!isValidCard(card)) {
        throw std::runtime_error("Invalid card play list is given.");
    }
}

Perform rearranging in place

To answer the question asked in a comment above the Game::sortByAlternatingScore(), yes, there is a nicer way to do it. Instead of allocating a separate vector, the operation can be done entirely in place:

void Game::sortByAlternatingScore() throw()
{
    std::sort(m_players.begin(), m_players.end(), [](const Player& p1, const Player& p2) {return p1.score > p2.score; });
    if (m_players.size() < 3) {
        return;
    }
    for (auto first=m_players.begin()+1; 
              first != m_players.end(); ++first) {
        std::reverse(first, m_players.end());
    }
}