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^^

• Simon Forsberg: Can you post a little summary that describes what kind of cardgame it is, or what the rules of it are?
• Kodnot: @SimonForsberg here you go^^

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());
 }
}

• Kodnot: Thanks for the amazing feedback! It'll take me a while to absorb all of the wisdom you've given me here! :D Just one question for now, where did I forget to include the <sstream> header? It's included in the game.cpp file, did I also use it somewhere else?
• Edward: You're right, I just overlooked the #include <sstream>. I've updated my answer to omit that incorrect comment.
• Kodnot: Could you please explain the logic behind your sortByAlternatingScore() function? I can't get it to work. It does work for the given example, but if you add a few extra players (I tested it with 11) it no longer works properly. I also tested it on a simple int array ({7, 6, 5, 4, 3, 2, 1, 0}). The output I get is {7, 0, 1, 2, 6, 5, 4, 3}, while what I should get is {7, 0, 6, 1, 5, 2, 4, 3}
• Edward: Sorry about that. I guess I only checked it with even numbers of items. It's fixed now, but I'm sure it's not the most elegant way to fix it.
• Edward: Hmm. Fixed, and this time with more thorough testing. It also inspired me to ask this question which might also be of interest to you if someone improves it significantly.