Need feedback on two member functions of a Table class in C++

Question

int Table::addPlayer(Player const& player, int position)
{
    if (position > 0 || position < 11) {
        deque<Player>::iterator it = playerList.begin()+position;
        deque<Player>::iterator itStart = playerList.begin()+postion;

        while(*it != "(empty seat)") {
            it++;
            if (it == playerList.end()) {
                it = playerList.begin();
            }
            if (it == itStart) {
cout << "Table full" << endl;
                return -1;
            }
        }
        //TODO overload Player assignment, << operator
        *it = player;
cout << "Player " << player << " sits at position " << it - playerList.begin() << endl;
            return it - playerList.begin();
    } else {
cout << "Position not a valid position, must be 1-10" << endl;
    return -1;
    }
}

int Table::removePlayer(Player const& player)
{
    for (deque<Player>::iterator it = playerList.begin();it != playerList.end(); it++) {
        //TODO Do I need to overload == in Player?
        if(*it == player) {
            *it = "(empty seat)";
            int pos = it - playerList.begin();
cout << "Player " << player << " stands up from position " << pos << endl;
            return pos;
        }
    }
cout << "Player " << player << " not found" << endl;
    return -1;
}

Would like some feedback on these two member functions of a Table class for Texas Hold Em Poker simulation. Any information syntax, efficiency or even common practices would be much appreciated.

Answer 1

Your first while loop in addPlayer() is dereferencing an iterator that hasn't been checked for validity. If a value of position is passed in that is greater than the number of elements in the container you will likely have a crash. This might be controlled by the caller, but it is much better practice to control it at the point of reference.

Answer 2

• Indent your code properly, it makes it far easier to read and understand later. If you're inexperienced, consider adopting a style guide (e.g. Google's C++ style guide).
• Checking that the player iterator dereferences to "(empty seat)" is questionable, you may want to consider a few alternatives:
  • Keep a separate list of empty chairs and allocate them on AddPlayer().
  • Let polymorphism fly and use a EmptySeatPlayer class.
  • Allow null players to be stored directly in the tableList.
• I'm unclear why AddPlayer needs a position parameter, if it just allocates the next available seat (until it finds one). Maybe remove the parameter entirely and let the Table figure it out.
• You'll eventually probably want to decouple your game-play text from the business logic.
• You probably shouldn't be using position directly, you should be operating on the players and the table. One might consider it a detail of the class that shouldn't be exposed by functions.
• Rather than while (*it != player) and checking for end in each iteration, use std::find.
• Also you're doing a lot of 'pass-by-value', it's good practice to pass const Player& to avoid unnecessary copies.

Answer 3

the remove could be done in a for loop..

for(deque<Player>::iterator it = playerList.begin(); it!= playerList.end(); it++){
    //if we've found what we're looking for
    if(*it == player){
     //then remove the player and return his/her position.
     *it = "(empty seat)";
     int pos = it - playerList.begin();
     cout << "Player " << player << " stands up from position " << pos << endl;
     return pos;
    }
}
cout << "Player " << player << " not found" << endl;
return -1;

I find this a bit cleaner, and I am personally a big fan of comments.

Answer 4

1) If you are not going to modify a parameter in a method then pass by const reference:

int Table::addPlayer(Player const& player, int position)

This provides hidden benfits latter but also introduces the concept of const correctness.

2) Try and learn how to use the standard algorithms:

In this case your loops can be replaced with the use of std::find()

int Table::addPlayer(Player const& player, int position)
{       
    deque<Player>::iterator itStart = playerList.begin()+position;

    deque<Player>::iterator it      = std::find(itStart, playerList.end(), "(empty seat)");
    if (it == playerList.end())
    {
        it  = std::find(playerList.begin(), itStart, "(empty seat)");
        if (it == itStart)
        {
            cout << "Table full" << endl;
            return -1;
        }
    }
    ...

And

int Table::removePlayer(Player const& player)
{
    deque<Player>::iterator it = std::find(playerList.begin(), playerList.end(), "(empty seat)");
    if (it == playerList.end())
    {
        cout << "Player " << player << " not found" << endl;
        return -1;
    }
    .....