trjhtr

For the purpose of becoming better at C++ programming, I wanted to program Ultimate Tic-Tac-Toe as a console application for Windows in C++. However, I thought it would be sensible to create the plain version of Tic-Tac-Toe first. So I made this simple program which I'll later extend/modify for the Ultimate version. I'll have some learning to do, but my goal is to write "flawless" code.

I'd like to know how I can improve my code, what some best practices are (that I'm omitting) and last but not least, what I'm already doing well.

main.cpp

#include "stdafx.h"
#include <iostream>
#include <windows.h>

constexpr int cols = 3;
constexpr int rows = 3;
constexpr char cellValueX = 'X';
constexpr char cellValueO = 'O';

/*
Clear console on Windows.
Code used from: https://stackoverflow.com/a/6487534/1964817
*/
void clearConsole()

 COORD topLeft = 0, 0;
 HANDLE console = GetStdHandle(STD_OUTPUT_HANDLE);
 CONSOLE_SCREEN_BUFFER_INFO screen;
 DWORD written;

 GetConsoleScreenBufferInfo(console, &screen);
 FillConsoleOutputCharacterA(console, ' ', 
 screen.dwSize.X * screen.dwSize.Y, 
 topLeft, &written);
 FillConsoleOutputAttribute(console, 
 FOREGROUND_GREEN 

class Player

private:
 char value;
 bool isAvailabletrue;

public:
 Player() 
 char getValue() return value; 
 bool operator==(const Player& compareTo) return value == compareTo.value; 
 void setValue(char newValue)
 
;

class Grid

private:
 class Cell
 
 private:
 int gridIndexGenerator()
 
 static int index0;
 return index++;
 ;
 const int idgridIndexGenerator() + 1;
 char valuestatic_cast<char>('0' + id);
 bool isAvailabletrue;

 public:
 Cell() 
 bool getIsAvailable() return isAvailable; 
 char getValue() return value; 
 void setValue(char newValue)
 ;
 ;
 enum WinDirection
 
 HORIZONTAL,
 VERTICAL,
 DIAGONAL,
 LAST
 ;

public:
 Grid() 
 Cell cells[rows][cols];
 void display();
 bool checkWin(char playerValue, WinDirection direction);
 bool checkWin(Player player);
;

void Grid::display()

 clearConsole();
 for(int row = 0; row < rows; row++)
 
 std::cout << std::endl;
 for(int col = 0; col < cols; col++)
 
 const char valuecells[row][col].getValue();
 std::cout << "[" << value << "]";
 
 

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


bool Grid::checkWin(char playerValue, WinDirection direction)

 switch(direction)
 
 case WinDirection::DIAGONAL:
 
 return ((cells[0][0].getValue() == playerValue &&
 cells[1][1].getValue() == playerValue &&
 cells[2][2].getValue() == playerValue) 
 case WinDirection::HORIZONTAL:
 case WinDirection::VERTICAL:
 
 bool result = false;
 for(int i = 0; i < rows; i++)
 
 switch(direction)
 
 case WinDirection::HORIZONTAL:
 
 result =
 (cells[i][0].getValue() == playerValue &&
 cells[i][1].getValue() == playerValue &&
 cells[i][2].getValue() == playerValue);
 break;
 
 case WinDirection::VERTICAL:
 
 result =
 (cells[0][i].getValue() == playerValue &&
 cells[1][i].getValue() == playerValue &&
 cells[2][i].getValue() == playerValue);
 break;
 
 
 if(result)
 
 return result;
 
 
 return result;
 
 


bool Grid::checkWin(Player player)

 bool resultfalse;
 char playerValueplayer.getValue();

 WinDirection direction = static_cast<WinDirection>(0);
 for(int i = 0; i != WinDirection::LAST; direction = static_cast<WinDirection>(++i))
 
 result = checkWin(playerValue, direction);
 if(result)
 
 return result;
 
 
 return result;


class Game

public:
 Game()
 
 playerX.setValue(cellValueX);
 playerO.setValue(cellValueO);
 player = playerX;
 
 void play();
 void switchPlayer();
 void getInput();

private:
 Player playerX;
 Player playerO;
 Player player;
 Grid gameGrid;
;

void Game::play()

 bool isGameWon = false;
 gameGrid.display();
 for(int i = 0; i < cols * rows && !isGameWon; i++)
 
 getInput();
 gameGrid.display();
 isGameWon = gameGrid.checkWin(player);

 if(isGameWon)
 
 std::cout << "Player " << player.getValue() << " won!" << std::endl;
 
 else switchPlayer(); 
 

 if(!isGameWon)
 
 std::cout << "It's a tie!" << std::endl;
 


void Game::switchPlayer()

 if(player == playerX) player = playerO; 
 else if(player == playerO) player = playerX; 


void Game::getInput()

 std::cout << "Your turn, Player " << player.getValue() << "!" << std::endl;

 int inputCell0;
 int col-1;
 int row-1;
 bool badInputfalse;
 bool isAvailablefalse;

 do
 
 while(badInput);

 gameGrid.cells[row][col].setValue(player.getValue());


int main()

 Game game;
 game.play();

• Organize your code. Separate your Windows API functions from your tic-tac-toe logic with different files. Place class definitions and public free-function prototypes into header files. Leave the implementation details in cpp files.




• Your Player class seems unnecessary. The entire functionality of the Player class (currently) is just to choose an X or an O. Just use variables instead.




• Your Cell class also seems unnecessary. Create a member-function in your Grid class that checks if a cell has an X or an O in it.




• The isAvailable variable should just be a function that checks if a cell contains an X or an O in it.




• Refactor it to not have global variables. The way you used global variables here is actually pretty good though. All of your globals variables are constants and have internal linking. The rows and cols variables could easily go into your Grid class. You could make them accessible to the Game class a variety of ways (public static variables, getters methods, make Game a friend class to Grid, etc). Similar strategies can be used for cellValueX and cellValueO. Another thing you could do is put everything into a namespace.




• Your nested switch statements are super ugly. Encapsulate different parts of the logic into separate functions (checkHorizonals(), checkVerticals(), checkDiagonals(), checkHorizontal(int row), checkVertical(int column), etc).




• Iterating through your enums is completely overkill for this problem. If you encapsulate checking your winning conditions into separate functions, you can simplify your logic and make it easier to read. Honestly, you could get rid of the enums altogther.




• Learn about const correctness. In particular, make sure your member-functions are const correct. This allows the compiler to make sure you're not modifying your class when you shouldn't be. It also makes it easy for others to see which functions change the state of an object which functions do not.




• Separate your game logic from your game presentation. Your Player and Grid classes do error handling by printing out a message to the standard output. Have them return bools or error codes or throw exceptions instead. Keep all of your writing to the console inside the Game class. Keep as much of your tic-tac-toe logic in your other classes. This way, if you ever decide to turn this into a game with a UI, all you have to do is replace the logic in Game.




• Instead of having your Grid::display() function, consider overloading operator<<. See the Stream extraction and insertion section on this site for details on how to do this.



• 
  

  Consider your final goal.

  
  
  

  You said you want to create an ultimate tic-tac-toe game. This means you will need to draw tic-tac-toe boards in a 3x3 fashion. Your ultimate tic-tac-toe logic will probably need to call a function that can print out individual rows from each tic-tac-toe board.

  
  
  

  Think about how you will refactor you code for ultimate-tic-tac-toe. I would not recommend changing your Grid class to have 81 cells. Instead I would recommend having 10 Grid objects (with the final one being the super board).

  
  
  

  You will also probably have to break your game loop into individual turns or steps since you will be switching between game boards almost every turn.

  
  

• You should be attempting to write portable code. I know why you used windows.h and don't have an immediate solution but it is something you will want to consider moving forward.




• Class defaults to private so you should not add it to the top of your class. It is however common practice to list public members first so that anyone using your class can read what they have access to without having to read over your private members.




• There are caveats that warn against using std::endl. The reasoning for this is that it flushes the stream and can be a performance hit. The counter argument (especially here) is that performance just isnt a concern in a small application. However I would further argue that you should try to understand what it is doing. Prefer "n" especially until you read up on flushing streams.




• You use a boolean switch isAvailable and check the values directly to determine if you are accidentally overwriting a players symbol. This is redundant. I would further suggest passing the char via constructor and initializing it and keeping it const. Then no overwriting.




• If you dont want to use the constructor then remove it. The compiler will generate one for you.




• Your enum is not scoped but you treat it as such. See enum class.




• Your cell should be a struct of it is a private construct of the grid class. It is not logically precarious for the grid to have full access to the Cells inner workings and Grid still encapsulates the cell and can expose only whats needed.




• You once again compare values while maintaining a bool when checking a Cell. Choose one.




• The underlying data structure of your Grid should not be public. In this case Cell cells[rows][cols]; You should also consider an std::array<> for added functionslity.




• Your player (read current or active) should be a pointer. Copying here might not be an expensive process but once again you should at least undertand whats happening. You also cant modify the original with your current approach. I believe a raw pointer would be acceptable here as there should be no ownership but merely acting as a reference pointer.