2
\$\begingroup\$

Looking for any feedback to improve practices on the following logic, and I have removed large chunks of documentation to reduce noise. I feel the areas that could expect heavy criticism are the win/tie condition checking.

Main.cpp

#include "Board.h"

int main() {
    //initialize board
    Board gameboard;

    for (int i = 0; i < gameboard.GetBoardSize(); i++) {
        gameboard.PlayTurn();
        gameboard.SetTurn(gameboard.GetTurn() + 1);
        if (gameboard.CheckWin()) {
            std::cout << gameboard.GetWin() << " wins!" << std::endl;
            break;
        }
        if (gameboard.CheckTie()) {
            std::cout << "Cat's Game!" << std::endl;
            break;
        }
        if (i == 8) {
            std::cout << "Cat's Game!" << std::endl;
            break;
        }
    }

    return 0;
}

Board.h

#pragma once

#include <iostream>
#include <string>
#include <vector>

class Board
{
private:
    std::vector<std::string> tiles;
    int turn;

public:
    //constructors

    Board();

    //getters

    std::string GetTile(int);
    int GetTurn();
    int GetBoardSize();

    //setters

    void SetTile(std::string, int);
    void SetTurn(int);

    //other methods

    void Display();

    void PlayTurn();

    bool CheckPlayedTile(int);

    bool CheckWin();

    bool CheckTie();

    std::string GetWin();
};

Board.cpp

#include "Board.h"

//constructors
Board::Board() {
    for (int i = 0; i < 9; i++)
        tiles.push_back(std::to_string(i));
    turn = 0;
    Display();
}

//getters
std::string Board::GetTile(int pos) {
    return tiles[pos];
}

int Board::GetTurn() {
    return turn;
}

int Board::GetBoardSize() {
    return tiles.size();
}

//setters
void Board::SetTile(std::string letter, int pos) {
    tiles[pos] = letter;
}

void Board::SetTurn(int newTurn) {
    turn = newTurn;
}

//other methods
void Board::Display() {
    std::cout << std::endl;

    if (turn == 0)
        std::cout << "Turn " << turn << std::endl;
    for (int i = 0; i < tiles.size(); i++) {
        if (i % 3 == 0 && i > 0)
            std::cout << std::endl << "---------" << std::endl;
        else if (i % 3 != 0 && i > 0)
            std::cout << " | ";
        std::cout << tiles[i];
    }

    std::cout << std::endl << std::endl;
}

void Board::PlayTurn() {
    int uiTile;

    if (turn % 2 == 0) {
        std::cout << "Turn " << turn << std::endl;
        std::cout << "Player X, please select a square: ";
        std::cin >> uiTile;
        if (!CheckPlayedTile(uiTile))
            tiles[uiTile] = "X";
        else {
            std::cout << uiTile << " has already been played, please select another" << std::endl;
            PlayTurn();
        }

    }
    else {
        std::cout << "Turn " << turn << std::endl;
        std::cout << "Player O, please select a square: ";
        std::cin >> uiTile;
        if (!CheckPlayedTile(uiTile))
            tiles[uiTile] = "O";
        else {
            std::cout << uiTile << " has already been played, please select another" << std::endl;
            PlayTurn();
        }
    }

    Display();
}

bool Board::CheckPlayedTile(int pos) {
    if (tiles[pos] == "X" || tiles[pos] == "O")
        return true;
    return false;
}

bool Board::CheckWin() {
    if (tiles[0] == tiles[1] && tiles[0] == tiles[2])
        return true;//tiles[0];
    else if (tiles[0] == tiles[3] && tiles[0] == tiles[6])
        return true;//tiles[0];
    else if (tiles[0] == tiles[4] && tiles[0] == tiles[8])
        return true;//tiles[0];
    else if (tiles[1] == tiles[4] && tiles[1] == tiles[7])
        return true;//tiles[1];
    else if (tiles[2] == tiles[5] && tiles[2] == tiles[8])
        return true;//tiles[2];
    else if (tiles[2] == tiles[4] && tiles[2] == tiles[6])
        return true;//tiles[2];
    else if (tiles[3] == tiles[4] && tiles[3] == tiles[5])
        return true;//tiles[3];
    else if (tiles[6] == tiles[7] && tiles[6] == tiles[8])
        return true;//tiles[6];
    return false;
}

bool Board::CheckTie() {
    for (int i = 0; i < tiles.size(); i++) {
        if (tiles[i] == "X" || tiles[i] == "O")
            return false;
    }

    return true;
}

std::string Board::GetWin() {
    if (tiles[0] == tiles[1] && tiles[0] == tiles[2])
        return tiles[0];
    else if (tiles[0] == tiles[3] && tiles[0] == tiles[6])
        return tiles[0];
    else if (tiles[0] == tiles[4] && tiles[0] == tiles[8])
        return tiles[0];
    else if (tiles[1] == tiles[4] && tiles[1] == tiles[7])
        return tiles[1];
    else if (tiles[2] == tiles[5] && tiles[2] == tiles[8])
        return tiles[2];
    else if (tiles[2] == tiles[4] && tiles[2] == tiles[6])
        return tiles[2];
    else if (tiles[3] == tiles[4] && tiles[3] == tiles[5])
        return tiles[3];
    else if (tiles[6] == tiles[7] && tiles[6] == tiles[8])
        return tiles[6];
    return "";
}
\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

The interface

When I call PlayTurn, I expect the turn counter to automatically increment.

gameboard.PlayTurn();
//gameboard.SetTurn(gameboard.GetTurn() + 1);

Rather than having the end-user do all the logic for the game's end, have a game-over check function. Try something like

bool Board::IsGameOver()
std::string Board::GameOverMessage()

so that the code will look like

if (gameboard.IsGameOver())
    std::cout << gameboard.GameOverMessage();

but I still suggest leaving in some of the interface you have, such as the "get winner" and other information state getters if the end-user wants to customize how the information is displayed.

Internal functions

Generally, you don't need to expose state-modifying functions publicly. For example, move functions like SetTurn to protected. This way, if the user ever decides to extend tic-tac-toe, he/she can still access the important state-modifying functions without the direct underlying members to mess things up.

Naming

Try to keep naming consistent. In some places, you use Hungarian notation and name variables uiTurn, and in others you do lower/UpperCamelCase. Pick something and stick to it.

Logic

Prefer member initialization over member assignment

Board::Board(): turn( 0 )
{
    for (int i = 0; i < 9; i++)
        tiles.push_back(std::to_string(i));
    Display();
}

Use an array over a vector if you don't need dynamic allocation.

std::array<std::string, 9> tiles;

for (int i = 0; i < tiles.size(); i++)
    tiles[i] = std::to_string(i);

CheckWin and GetWin can be cleaned up by implementing a new function.

int WinningRows[8][3]{
{0,1,2}, // horizontal wins
{3,4,5},
{6,7,8},
{0,3,6}, // vertical wins
{1,4,7},
{2,5,8},
{0,4,8}, // diagonal wins
{2,4,6}
};


bool Board::WinningRow(int * rows)
{
    return tiles[rows[0]] == tiles[rows[1]]
           && tiles[rows[0]] == tiles[rows[2]];
}

std::string Board::GetWin() {
    for (int i = 0; i < 8; ++i)
        if (WinningRow(WinningRows[i]))
            return GetTile(WinningRows[i][0]);
    return "";
}

bool Board::CheckWin() {
    for (int i = 0; i < 8; ++i)
        if (WinningRow(WinningRows[i]))
            return true;
    return false;
}

Interface functions should abstract implementation details. Rather than numbering each square 0-8 because you have a continuous array, make it take two arguments: x and y.

std::string Board::GetTile(int x, int y) {
    return tiles[3*y + x];
}

CheckTie does not make sense to me based on the name -- it looks as though it checks if the board is empty. What does your documentation say? What is its purpose? I'll, for now, assume it is correct.

\$\endgroup\$
0

You must log in to answer this question.