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

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 = p开发者_运维问答layerList.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.

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.

• 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.

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.

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;
    }
    .....