4
\$\begingroup\$

I was told I need code in order for someone to review my UML Class Diagram for my Noughts and Crosses console application. I would like to know if it's intuitive, whether you could make a program based on it, and whether any improvements need to be made on it. UML class diagram

    #include <iostream>
    #include <vector>
    #include <ctime>
    #include <cstdlib>       
    #include <algorithm>
    #include <random>
    #include <chrono>
    #include <array>

class Board {
    std::array<unsigned char, 9> m_board;
    std::vector<int>m_validLocations;
public:
    Board();
    auto& get();
    void shuffleValidLocations();
    bool isMoveValid(int move);
    auto& getValidLocation();
    void reset();
    void display() const;
    void removeFromValidLocation(int move);
    ~Board() = default;
};


class Player {
public:
    
    unsigned char m_symbol;
    const std::string m_type;
    int m_wins;
    int m_draws;
    virtual int nextMove(Board& board)  = 0;
    Player(const unsigned char symbol, std::string&& type);
    virtual ~Player() = default;
};

class Human : public Player {
public:
    Human(unsigned char symbol) :Player{ symbol, "Human" } {}
    virtual int nextMove(Board& board)  override;
    virtual ~Human() = default;
};
class Robot : public Player {
public:
    Robot(unsigned char symbol) :Player{ symbol, "Robot" } {}
    virtual int nextMove(Board& board)  override;
    virtual ~Robot() = default;
};
class NoughtsAndCrosses
{
    Player* m_one;
    Player* m_two;
    Player* m_turn;
    Player* m_winner;
    Board m_board;

    bool draw();
    void playerMove();
    void switchPlayers();
    bool win();
public:

    NoughtsAndCrosses(Player& one, Player& two);
    Player* playerWithMostWins();
    Player* play();
    ~NoughtsAndCrosses() = default;
};

auto& Board::get() { return m_board; }
Board::Board()
{
    m_board = { '-', '-', '-', '-', '-', '-', '-', '-', '-', };
    m_validLocations = { 1,2,3,4,5,6,7,8,9 };
}
void Board::shuffleValidLocations() {
    auto seed = std::chrono::system_clock::now().time_since_epoch().count();
    std::default_random_engine e(seed);
    std::shuffle(m_validLocations.begin(), m_validLocations.end(), e);
}
bool Board::isMoveValid(int move) {
    return (std::any_of(m_validLocations.begin(), m_validLocations.end(), [&](int& num) {return num == move; }));
}
auto& Board::getValidLocation() { return m_validLocations; }
void Board::reset()
{
    std::fill(m_board.begin(), m_board.end(), '-');
    m_validLocations = { 1,2,3,4,5,6,7,8,9 };
}
void Board::display() const
{
    int num = 0;
    for (auto const& cell : m_board)
    {
        num++;
        if (num % 3 == 1) {
            std::cout << "\n\n";
        }
        if (cell != '-') {
            std::cout << cell << "        ";
        }
        else
        {
            std::cout << num << "        ";
        }
    }
    std::cout << "\n\n";
}
void Board::removeFromValidLocation(int move) {
    std::remove_if(m_board.begin(), m_board.end(), [&](auto& number) {
        return number == move;
        });
}

Player::Player(const unsigned char symbol, std::string&& type)
    :m_symbol{ symbol }, m_type{ type }, m_wins{ 0 }, m_draws{ 0 } {}
 int Human::nextMove(Board& board) {
    int move = 0;
    bool isMoveValid = false;
    do
    {
        std::cout << "Enter a number on the board (e.g. 1): ";
        std::cin >> move;
        if (!(isMoveValid = board.isMoveValid(move)))
        {
            std::cout << "Invalid move! Choose a valid location!\n";
        }
    } while (!isMoveValid);
    board.removeFromValidLocation(move);
    return move - 1;
}

 int Robot::nextMove(Board& board)   {
    int move = 0;
    board.shuffleValidLocations();
    move = board.getValidLocation().at(0);
    board.getValidLocation().erase(board.getValidLocation().begin());
    return move - 1;
}

NoughtsAndCrosses::NoughtsAndCrosses(Player& one, Player& two)
    :m_one(&one), m_two(&two), m_turn(&one), m_winner(nullptr) {}
bool NoughtsAndCrosses:: draw() {
    return std::all_of(m_board.get().begin(), m_board.get().end(), [&](auto& pair) {return pair != '-'; });
}
void NoughtsAndCrosses::playerMove()
{
    m_board.get().at(m_turn->nextMove(m_board)) = m_turn->m_symbol;
}
void NoughtsAndCrosses::switchPlayers()
{
    m_turn = m_turn == m_one ? m_two : m_one;
}
bool NoughtsAndCrosses::win() {
    if (m_board.get().at(0) == m_turn->m_symbol && m_board.get().at(1) == m_turn->m_symbol && m_board.get().at(2) == m_turn->m_symbol) {
        m_winner = m_turn;
        return true;
    }
    else if (m_board.get().at(3) == m_turn->m_symbol && m_board.get().at(4) == m_turn->m_symbol && m_board.get().at(5) == m_turn->m_symbol) {
        m_winner = m_turn;
        return true;
    }
    else if (m_board.get().at(6) == m_turn->m_symbol && m_board.get().at(7) == m_turn->m_symbol && m_board.get().at(8) == m_turn->m_symbol) {
        m_winner = m_turn;
        return true;
    }
    else if (m_board.get().at(0) == m_turn->m_symbol && m_board.get().at(3) == m_turn->m_symbol && m_board.get().at(6) == m_turn->m_symbol) {
        m_winner = m_turn;
        return true;
    }
    else if (m_board.get().at(1) == m_turn->m_symbol && m_board.get().at(4) == m_turn->m_symbol && m_board.get().at(7) == m_turn->m_symbol) {
        m_winner = m_turn;
        m_winner->m_wins++;
        return true;
    }
    else if (m_board.get().at(2) == m_turn->m_symbol && m_board.get().at(5) == m_turn->m_symbol && m_board.get().at(8) == m_turn->m_symbol) {
        m_winner = m_turn;
        return true;
    }
    else if (m_board.get().at(0) == m_turn->m_symbol && m_board.get().at(4) == m_turn->m_symbol && m_board.get().at(8) == m_turn->m_symbol) {
        m_winner = m_turn;
        return true;
    }
    else if (m_board.get().at(6) == m_turn->m_symbol && m_board.get().at(4) == m_turn->m_symbol && m_board.get().at(2) == m_turn->m_symbol) {
        m_winner = m_turn;
        return true;
    }
    else
    {
        return false;
    }
}
Player* NoughtsAndCrosses::playerWithMostWins() {
    return m_one->m_wins > m_two->m_wins ? m_one : m_two;
}
Player* NoughtsAndCrosses::play() {
    while (true)
    {
        m_board.display();
        playerMove();
        if (win())
        {
            std::cout << m_winner->m_symbol << " is the winner!\n";
            break;
        }
        else if (draw())
        {
            m_winner = nullptr;
            break;
        }
        switchPlayers();
    }
    m_board.display();
    m_board.reset();
    m_turn = m_one;
    return m_winner;
}


int main()
{
    Robot robot1('X');
    Robot robot2('O');

    //pass these in to NoughtsAndCrosses if you want to play human vs human, or one if you want to play human vs Robot
    Human human1('X');
    Human human2('O');
  
    NoughtsAndCrosses game(robot1, robot2);
    int round = 3;
    int roundCount = 0;
    Player* winner = nullptr;
    do
    {
        int gameCount = 1;
        int totalGamesinRound = 3;
        std::cout << "START GAME!\n";
        system("pause");
        system("cls");
        std::cout << "\nROUND " << ++roundCount << ". . .\n";
        do
        {
            std::cout << "Game " << gameCount << " of round " << roundCount << "\n";
            winner = game.play();

            if (winner != nullptr)
            {
                std::cout << "Winner of game " << gameCount << " is type: " << winner->m_type << ": " << winner->m_symbol << "\n";
                winner->m_wins++;
            }
            else
            {              
                std::cout << "Game " << gameCount << " is a draw!\n";  
                robot1.m_draws++;
                robot2.m_draws++;
            }
            gameCount++;
            totalGamesinRound--;
        } while (totalGamesinRound != 0);

        /* std::cout << "Game 2: Human vs Robot\n";
         game.play(robot1, robot1);*/
        std::cout << "Wins for " << robot1.m_type << ": Player : " << robot1.m_symbol << " - " << robot1.m_wins << "\n";
        std::cout << "Wins for " << robot2.m_type << ": Player : " << robot2.m_symbol << " - " << robot2.m_wins << "\n";
        std::cout << "Drawed: " << robot1.m_draws << "\n";

        std::cout << "Winner of round " << roundCount << " is " << game.playerWithMostWins()->m_symbol << "\n";
        round--;
    } 
    while (round != 0);
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ This is really a nice design. :-) \$\endgroup\$ Dec 15 '20 at 6:18
  • 1
    \$\begingroup\$ Thank you very much! \$\endgroup\$ Dec 15 '20 at 8:44
  • \$\begingroup\$ So what happens when two x players play each other in a game? Shouldn't symbols be defined only for the duration of a single game? But players seem to persist longer since they have win/draw counts. \$\endgroup\$ Dec 15 '20 at 15:01
  • \$\begingroup\$ I designed it to be like a tournament hence the win and draw counts. I probably should have given the players a name e.g. Fred as well as a symbol then allow them to take turns being O and X. \$\endgroup\$ Dec 15 '20 at 15:27
  • \$\begingroup\$ Also, I wouldn’t allow two X players to play against each other. \$\endgroup\$ Dec 15 '20 at 15:29
4
\$\begingroup\$

Upvote, what a nice and simple UML Design!

Simplicity is a step to archive scalability, maintainability and ensuring software quality.

Concerning the UML model:

  • You must use the composition specifier for the relationship between Board and NoughtsAndCrosses since the instance of Board must be provided so that UB is prevented.

  • The relation between Player and NoughtsAndCrosses is more likely to be composition or aggregation. Why? Because NoughtsAndCrosses requires on its constructor two references to initialize the objects p1 and p2. Although, one may note that m_pTurn changes, it has the property to be initialized by either p1 or p2. Finally, m_pWinner is quite special, it should be a nullptr_t pointer until the winner is known.

  • Respect the definition of Player and the inheritance of Robot and Human well, it's quite strange (at least for me) that you are overriding just one member function. You could instead use an state pattern like solution.

Consider: std::functional<int(Board&)> next_move; as a class attribute, you could define two functions or two lambdas so that next_move is interchangeable and you're saving code. (Just a suggestion)

Concerning your code

You could use in-class definitions for your functions instead of prototyping and then defining them. (Make less your efforts)

Using auto is good for iterating values, however, it may lead to type inconsistencies in greater schemes. When in Roma, do as Romans do. If you're working with C++ which has a strict type system, attempt to make use of clear and consistent typing.

Thank for reading this answer, other users may have more in deep comments for your code. For further questions regarding UML modeling you could go to Software Engineering Stackexchange there you could find design related Q&A.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Wow! Thanks so much! I really appreciate your feedback! Thanks for that suggestion it has given me lots to think about! \$\endgroup\$ Dec 15 '20 at 8:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.