Custom card game

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
#include
#include
#include
#include
#include
#include

// 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 m_players;

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

// Cards in game, game ends when no more cards ar

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 har

StackExchange Code Review Q#123713, answer score: 4