Нужна обратная связь по двум функциям-членам класса Table в C ++ - PullRequest
Новая
       10

Нужна обратная связь по двум функциям-членам класса Table в C ++

1 голос
/ 02 июня 2010 
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;
}

Хотелось бы получить отзывы об этих двух функциях класса Table для симулятора Texas Hold Em Poker. Любой синтаксис информации, эффективность или даже обычные практики будут высоко ценится.

Ответы [ 4 ]

2 голосов
/ 02 июня 2010

Ваш первый цикл while в addPlayer () разыменовывает итератор, который не был проверен на достоверность. Если передано значение position, которое больше, чем количество элементов в контейнере, у вас, скорее всего, произойдет сбой. Это может контролироваться вызывающим абонентом, но гораздо эффективнее контролировать его в исходной точке.

0 голосов
/ 02 июня 2010

1) Если вы не собираетесь изменять параметр в методе, тогда передайте константную ссылку: 

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

Это обеспечивает скрытые преимущества последней, но также вводит концепцию правильности const.

2) Попробуйте и научитесь использовать стандартные алгоритмы:

В этом случае ваши циклы можно заменить с использованием 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;
        }
    }
    ...

И 

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;
    }
    .....
0 голосов
/ 02 июня 2010

удаление может быть сделано в цикле for. 

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;

Я считаю, что это немного чище, и я лично большой поклонник комментариев.

0 голосов
/ 02 июня 2010
  • Сделайте отступ в своем коде правильно, это облегчит чтение и понимание позже. Если вы неопытны, рассмотрите возможность принятия руководства по стилю (например, Руководство по стилю Google C ++ ).
  • Проверяя, что итератор игрока разыменовывает "(свободное место)", сомнительно, вы можете рассмотреть несколько альтернатив:
    • Вести отдельный список пустых стульев и распределить их по AddPlayer().
    • Пусть полиморфизм летит и использует класс EmptySeatPlayer.
    • Разрешить сохранение null игроков непосредственно в tableList.
  • Мне неясно, зачем AddPlayer нужен параметр position, если он просто распределяет следующее доступное место (пока не найдет его). Возможно, удалите параметр полностью и дайте Table понять его.
  • Возможно, вы захотите отделить игровой текст от бизнес-логики.
  • Вы, вероятно, не должны использовать position напрямую, вы должны работать с игроками и столом. Кто-то может считать это деталью класса, которая не должна быть раскрыта функциями.
  • Вместо while (*it != player) и проверки конца в каждой итерации, используйте std::find.
  • Кроме того, вы делаете много «передачи по значению», рекомендуется передавать const Player&, чтобы избежать ненужных копий.
...