C ++ OOP Tic Tac Toe
Dies ist eine Fortsetzung meiner Frage hier . Nun, es ist nicht gerade ein Follow-up, sondern eher mein nächstes Projekt nach dem letzten
Ich habe ein Tic Tac Toe-Spiel mit objektorientierter Programmierung erstellt
Sie alle wissen bereits, wie der Tic Tac Toe funktioniert, damit ich Ihre Zeit nicht damit verschwenden werde, Ihnen zu erklären, wie er funktioniert
Ich suche Feedback zu absolut allem, was mich zu einem besseren Programmierer machen könnte, insbesondere zu einem besseren C ++ - Programmierer, auch dazu, wie man Klasse verwendet, besser funktioniert und wie man OOP richtig verwendet, und diese:
- Optimierung
- Schlechte und gute Praxis
- Codestruktur
- Funktionen und Variablennamen
- Bugs
- Verbesserung der Klassen- und Funktionsnutzung
- So verwenden Sie OOP richtig
- Oh, auch wie man einen Kommentar richtig hinzufügt
- usw
Vielen Dank!
Ich verwende Visual Studio Community 2019 Version 16.7.7
Globals.h
#ifndef GUARD_GLOBALS_H
#define GUARD_GLOBALS_H
namespace
{
enum class Players : char
{
PLAYER_X = 'X',
PLAYER_O = 'O'
};
}
#endif // !GUARD_GLOBALS_H
board.h
#ifndef GUARD_BOARD_H
#define GUARD_BOARD_H
#include "player.h"
class Board
{
private:
char board[9];
// This is suppose to be a place to put the score
// But I don't know how to implement it yet
int scoreX{};
int scoreO{};
public:
Board();
void printBoard() const;
void markBoard(const size_t& choseNum, const char& player, bool& inputPass);
char checkWin(bool& isDone, int& countTurn);
void printWinner(bool& isDone, int& countTurn);
};
#endif // !GUARD_BOARD_H
board.cpp
#include "board.h"
#include <iostream>
// To set the board with numbers
Board::Board()
{
int j{ 1 };
for (int i = 0; i < 9; i++)
{
board[i] = '0' + j++;
}
}
void Board::printBoard() const
{
system("cls");
std::cout << " | | " << "\n";
std::cout << " " << board[0] << " | " << board[1] << " | " << board[2] << "\tPlayer X: " << scoreX << "\n";
std::cout << "___|___|__" << "\tPlayer O: " << scoreO << "\n";
std::cout << " | | " << "\n";
std::cout << " " << board[3] << " | " << board[4] << " | " << board[5] << "\n";
std::cout << "___|___|__" << "\n";
std::cout << " | | " << "\n";
std::cout << " " << board[6] << " | " << board[7] << " | " << board[8] << "\n";
std::cout << " | | " << "\n\n";
}
// To change the board to which the player choose the number
void Board::markBoard(const size_t& choseNum, const char& player, bool& inputPass)
{
char checkNum = board[choseNum - 1];
// To check if the number that the player choose is available or not
if (checkNum != (char)Players::PLAYER_X && checkNum != (char)Players::PLAYER_O)
{
// To check if the number that the player input
if (choseNum >= 1 && choseNum <= 9)
{
board[choseNum - 1] = player;
inputPass = true;
}
else
{
std::cout << "CHOOSE THE AVAILABLE NUMBER!\nTRY AGAIN: ";
}
}
else
{
std::cout << "SPACE HAS ALREADY BEEN OCCUPIED\nTry again: ";
}
}
/*
There is probably a better way to do this. But, I don't know how tho
Maybe someday I could improve the checking for win but right now
this is good enough
Also, there are a lot of magic number here such as 8, 2, 6 and 7.
I've tried to remove the magic number but I don't know how.
*/
// Check the board if there is player with parallel set or not
char Board::checkWin(bool &isDone, int &countTurn)
{
/*
I use middleboard and initialize it to board[4] because in order
for a player to win diagonally they have to acquire the
middle board first. So, I initialize middleboard to board[4]
hoping it could remove the magic number
and I initialize i to 0 and j to 8 because the checking is
begin from the top left corner-middle-bottom right corner
if it false then I add add 2 to i and substract 2 from j
because now the checking is top right corner-middle-bottom left corner
*/
// Check diagonal win
size_t middleBoard = board[4];
for (size_t i = 0, j = 8; i <= 2 && j >= 6; i+=2, j-=2)
{
// If all the board is occupied by the same player then the same player win
if (middleBoard == board[i] && board[i] == board[j])
{
//This is suppose to add score, but I don't know how to implement it yet
board[middleBoard] == (char)Players::PLAYER_X ? scoreX++ : scoreO++;
isDone = true;
return middleBoard; // To return the character of the player who won
}
}
/*
I initialize initialNum to 0 as a starting point for the checking.
Initilialized i to 1 and j to 2
The checking is like this, top left corner-middle top-top right corner
If it false then the I add 3 to initialNum to make middle left as the
starting point, then add 3 to i and j so it the next checking is
middle left-middle-middle right, and so on
*/
// Check horizontal win
size_t initialNum = 0;
for (size_t i = 1, j = 2; i <= 7 && j <= 8; i += 3, j += 3)
{
if (board[initialNum] == board[i] && board[i] == board[j])
{
board[initialNum] == (char)Players::PLAYER_X ? scoreX++ : scoreO++;
isDone = true;
return board[initialNum];
}
else
{
initialNum += 3;
}
}
/*
I reset the initialNum to 0 and initialized i to 3 and j 6 so
the first check will be like this: top left corner-middle left-bottom left corner
if it fails then i add 1 to initialNum, i, and j, so the next check will be
middle top-middle-middle bottom and so on
*/
// Check vertical win
initialNum = 0;
for (size_t i = 3, j = 6; i <= 5 && j <= 8; i++, j++)
{
if (board[initialNum] == board[i] && board[i] == board[j])
{
board[initialNum] == (char)Players::PLAYER_X ? scoreX++ : scoreO++;
isDone = true;
return board[initialNum];
}
else
{
initialNum++;
}
}
// If the countTurn is 8 then there're no place to occupy anymore, thus a draw
if (countTurn == 8)
{
isDone = true;
return 'D'; // As a check for printWinner() function
}
countTurn++;
}
// To print who's the winner or draw
void Board::printWinner(bool& isDone, int& countTurn)
{
if (checkWin(isDone, countTurn) == 'D')
{
std::cout << "It's a Draw!\n";
}
else
{
std::cout << "Congratulations!\nPlayer " << checkWin(isDone, countTurn) << " won the game!\n";
}
}
player.h
#ifndef GUARD_PLAYER_H
#define GUARD_PLAYER_H
#include "Globals.h"
#include "board.h"
class Board;
class Player
{
private:
char mainPlayer;
char secondPlayer;
char turnPlayer = mainPlayer;
public:
void choosePlayer(bool &choosePass);
void movePlayer(Board& myBoard);
void switchPlayer();
};
#endif // !GUARD_PLAYER_H
player.cpp
#include "player.h"
#include "board.h"
#include <iostream>
#include <random>
// To give a choice for the player if they want to be X or O
void Player::choosePlayer(bool& choosePass)
{
char chosePlayer;
std::cout << "Do you want to be player X or O? ";
while (!choosePass)
{
std::cin >> chosePlayer;
// If the player type X uppercase or lowercase then they will be
// X and the computer will be O, vice versa
if (chosePlayer == 'x' || chosePlayer == 'X')
{
mainPlayer = (char)Players::PLAYER_X;
secondPlayer = (char)Players::PLAYER_O;
choosePass = true;
}
else if (chosePlayer == 'o' || chosePlayer == 'O')
{
mainPlayer = (char)Players::PLAYER_O;
secondPlayer = (char)Players::PLAYER_X;
choosePass = true;
}
else
{
std::cout << "Invalid choice\n Try again: ";
}
}
}
// To make a player choose a number to which they want to occupy
void Player::movePlayer(Board &myBoard)
{
size_t choseNum;
bool inputPass = false;
/*
I make it turnPlayer != mainPlayer because if I make it
turnPlayer == mainPlayer then the computer will make the first move
I don't know why. Probably should find out the why. But it'll do for now
*/
// If turnPlayer is not mainPlayer then it's the player's move
if (turnPlayer != mainPlayer)
{
std::cout << "Player " << mainPlayer << " choose a number: ";
while (!inputPass)
{
if (std::cin >> choseNum)
{
myBoard.markBoard(choseNum, mainPlayer, inputPass); //Go to markBoard function in board.cpp
}
else
{
std::cout << "Invalid input type (Type only number)\nTry again: ";
std::cin.clear(); // To clear the input so
std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n'); // the player can input again
}
}
}
// If the turnPlayer is mainPlayer then it's the computer's move
else
{
while (!inputPass)
{
// To make a random move for the computer
std::random_device rd;
std::mt19937 gen(rd());
std::uniform_int_distribution<> distrib(1, 9);
choseNum = distrib(gen);
myBoard.markBoard(choseNum, secondPlayer, inputPass);
}
}
}
// To change turn, if the player finishes then the computer will make the move
void Player::switchPlayer()
{
turnPlayer = (turnPlayer == mainPlayer) ? secondPlayer : mainPlayer;
}
main.cpp
#include "board.h"
#include "player.h"
int main()
{
Board myBoard;
Player mainPlayer;
int countTurn{ 0 };
bool choosePass = false;
bool isDone = false;
myBoard.printBoard(); // To print the initial board with numbered spaces
while (!isDone)
{
if (!choosePass)
{
mainPlayer.choosePlayer(choosePass);
}
mainPlayer.movePlayer(myBoard);
myBoard.printBoard();
mainPlayer.switchPlayer();
myBoard.checkWin(isDone, countTurn);
}
myBoard.printWinner(isDone, countTurn);
}
Antworten
Sollte es geben Globals.h?
Ich bin nicht einverstanden. Globals.hhat eine Single enum, die nur für Ihre PlayerKlasse von Bedeutung ist. Warum also einen neuen Header erstellen? Warum kann man nicht enum class Playerseinfach dabei sein Player.cpp? Dies ist die einzige Datei, die jemals auf den Inhalt von zugreift Players. Ich glaube, das Beste, was Sie hier tun können, ist, einen anonymen NamespacePlayer.cpp zu erstellen und dort zu lassen.
// Player.cpp
namespace {
enum class Players { ... };
}
Seien Sie auch vorsichtig, wenn Sie einen unbenannten Namespace in einer Header-Datei verwenden
Verwenden Sie std :: tolower
Anstatt mit beiden Fällen eines Zeichens zu vergleichen, std::tolowerkonvertieren Sie ein Zeichen direkt in Kleinbuchstaben. Dies würde konvertieren
std::cin >> chosePlayer;
if (chosePlayer == 'x' || chosePlayer == 'X') {...}
else if (chosePlayer == 'o' || chosePlayer == 'O') {...}
else {...}
In
std::cin >> chosePlayer;
chosePlayer = std::tolower(chosePlayer, std::locale());
if (chosePlayer == 'x' ) {...}
else if (chosePlayer == 'o') {...}
else {...}
#include <locale>
- Beachten Sie, dass der Code bei Eingabe von mehr als 1 Zeichen das erste akzeptiert. Wenn der Benutzer beispielsweise eingibt
cplusplus,chosePlayerwird jetzt auf gesetztc.
Verwenden enum classSie die von Ihnen erstellte
Du hast ein enumEntfernen der Magie xund erstellt o. Warum benutzt du sie hier immer noch?
if (chosePlayer == 'x' )
else if (chosePlayer == 'o')
Verwenden Sie die Werte von enum class Playershier.
Verwenden Sie ein enumhier
Einige mögen anderer Meinung sein, aber ich denke, ein enumist besser als enum classhier. Der Grund dafür ist, dass Sie die Werte nicht ständig umwandeln müssen, charwann immer Sie einen enumund einen charTyp vergleichen möchten .
Wenn es nur in einer einzelnen .cppDatei sichtbar sein soll, wie ich bereits erwähnt habe, treten höchstwahrscheinlich keine Namenskonflikte auf.
enum Player : char { PLAYER_1 = 'x', PLAYER_2 = 'o' };
Von Player::chosePlayer()
void Player::choosePlayer(bool& choosePass)
{
char chosePlayer;
std::cout << "Do you want to be player X or O? ";
while (!choosePass)
{
std::cin >> chosePlayer;
// If the player type X uppercase or lowercase then they will be
// X and the computer will be O, vice versa
if (chosePlayer == 'x' || chosePlayer == 'X')
{
mainPlayer = (char)Players::PLAYER_X;
secondPlayer = (char)Players::PLAYER_O;
choosePass = true;
}
else if (chosePlayer == 'o' || chosePlayer == 'O')
{
mainPlayer = (char)Players::PLAYER_O;
secondPlayer = (char)Players::PLAYER_X;
choosePass = true;
}
else
{
std::cout << "Invalid choice\n Try again: ";
}
}
}
Wenn Sie angeben möchten, ob die eingegebenen Werte gut oder schlecht sind, warum übergeben Sie einen Verweis auf eine boolVariable? Warum nicht zurückkehren, truewenn die Eingabe gut ist und falsewenn die Eingabe nicht gut ist? Wenn Sie als Referenz übergeben, wird implizit ein Zeiger übergeben, sodass Sie tatsächlich einen Zeiger an eine Bool-Variable in der Funktion übergeben. Sie müssen als Referenz übergeben, wenn Sie mit Ihrer aktuellen Logik gehen, aber die Sache ist
sizeof(bool) == 2
sizeof(bool*) == 8
Aus diesem Grund und der Einfachheit halber glaube ich, einfach zurückzukehren Trueoder Falsebesser zu sein
Nach einem Gewinner suchen
Ihr aktueller Algorithmus zur Suche nach einem Gewinner ist sehr lang und schwer zu lesen. Es gibt bessere Wege. Dieser Thread enthält viele nützliche Informationen dazu . Das einfachste von allen
constexpr int NB_WIN_DIR = 8;
constexpr int N = 3; // please think of a better name
constexpr int wins[NB_WIN_DIR][N] {
{0, 1, 2}, // first row
{3, 4, 5}, // second row
{6, 7, 8}, // third row
{0, 3, 6}, // first col
{1, 4, 7}, // second col
{2, 5, 8}, // third col
{2, 4, 6}, // diagonal
{0, 4, 8}, // antidiagonal
};
for (int i = 0; i < NB_WIN_DIR ;i++)
{
if (board[wins[0]] == board[wins[1]] and board[wins[1]] == board[wins[2]])
return board[wins[0]];
}
Wann sollten Sie vorbeikommen const&?
Ich sehe ein const bool&und const size_t&Funktionsargumente.
Wann sollten Sie als konstante Referenz übergeben
- Wenn Sie Kopien für große Objekte vermeiden möchten
Wie ich bereits sagte, wird beim Übergeben als Referenz implizit ein Zeiger übergeben. Aber das Problem ist
sizeof(bool) == 2
sizeof(bool*) == 8
sizeof(size_t) == 8 // depending on your machine, sometimes 4
sizeof(size_t*) == 8
Im besten Fall tut es dir überhaupt nicht gut und es tut dir möglicherweise mehr schlecht . Als einfache Faustregel müssen Sie keine primitiven Typen wie int, char, double, floatby übergeben const&, sondern nur als Referenz, wenn Sie so etwas haben std::vector.
Verstehen Sie mich nicht falsch, Sie sollten als Referenz übergeben, wenn eine Funktion den ursprünglichen Wert eines Objekts ändern soll. Wenn dies jedoch nicht beabsichtigt ist, verwenden Sie es nur für große Objekte.
Überdenken Sie Ihre Codestruktur
Ich mag diese Klasse wirklich nicht
class Player
{
private:
char mainPlayer;
char secondPlayer;
char turnPlayer = mainPlayer;
public:
void choosePlayer(bool &choosePass);
void movePlayer(Board& myBoard);
void switchPlayer();
};
Ihre PlayerKlasse enthält keine Informationen über einen einzelnen Spieler. Alle Ihre Mitgliedsfunktionen ändern die Werte Ihrer board. All dies gehört tatsächlich zu Ihrer BoardKlasse. Ein Spieler ist eigentlich nur ein char, entweder ooder x. Es enthält buchstäblich keine anderen Informationen als diese. Was Sie tun sollten, ist einfach einen Spieler mit einer Aufzählung darzustellen, wie Sie es bereits getan haben
enum Player { ... };
class Board{
Player human;
Player bot;
};
Das botwäre der Computer, der gegen Sie spielt, und humanwäre der eigentliche Benutzer.
Was denken, sollte mit einer Klasse dargestellt werden, ist ein einfacher Schritt. Ein Zug hat zwei Dinge.
- Das Quadrat
- Der Spieler
Überall in Ihrem Programm haben Sie diese beiden getrennt weitergegeben. Warum nicht ein einfaches erstellen struct, das es enthält?
struct Move {
int square;
Player player;
}
Ich habe ein sehr einfaches Beispiel dafür geschrieben, wie dieses Spiel neu strukturiert werden kann.
class Game
{
private:
struct Move {
Player player;
int square;
Move(const int square, const Player player)
: square(square), player(player)
{}
};
enum Player {
PLAYER_1, PLAYER_2, NONE
};
template < typename T, size_t N > using array = std::array < T, N >;
array < char, NB_SQ > board;
Player human;
Player bot;
short int turns; // number of total moves played
void computer_move();
Move input_move() const;
void make_move(const Move& move);
bool validate_move(const Move& move);
Player check_win() const;
bool check_draw() const;
void print_board() const;
void new_game(); // choose whether the player plays 'x' or 'o' here
public:
void mainloop(){
for (;;) {
const Move& move = input_move();
make_move(move);
computer_move();
if (check_win()) // ...
if (check_draw()) // ...
}
}
Game() { new_game(); }
};
int main() {
Game game;
game.mainloop();
}
Über system("cls")
Ihr aktuelles Programm funktioniert nicht auf Betriebssystemen, die keine Windows sind. Auf den meisten anderen Systemen lautet das Wort clear. Um dies portabler zu machen, können Sie eine # ifdef- Anweisung verwenden, um nach dem Betriebssystem zu suchen
void clear_screen()
{
#ifdef _WIN32
system("cls");
#else
system("clear");
#endif
}
Weiterlesen
Allgemeine Beobachtungen
Der Code main()ist gut dimensioniert, schön und eng, sehr gut lesbar. Der einzige Nachteil main()ist der Kommentar, der wirklich nicht notwendig ist.
Es scheint gegenseitige Abhängigkeiten zwischen Board und Player zu geben. Beim Software-Design ist dies als enge Kopplung bekannt und weist im Allgemeinen auf ein schlechtes Design hin.
Ich sehe nur eine Instanz der Player-Klasse und ich würde 2 Instanzen erwarten, eine für jeden Spieler.
Arbeiten Sie weiter an Ihren Objektdesigns, um die enge Kopplung zu beseitigen, und versuchen Sie, die SOLID- Programmierprinzipien zu befolgen . Lernen Sie einige objektorientierte Entwurfsmuster wie Komposition kennen.
SOLID ist eine Abkürzung für fünf Designprinzipien, die Software-Designs verständlicher, flexibler und wartbarer machen sollen. Auf diese Weise können Sie Ihre Objekte und Klassen besser gestalten.
- Das Prinzip der Einzelverantwortung - Eine Klasse sollte nur eine Einzelverantwortung haben, dh nur Änderungen an einem Teil der Softwarespezifikation sollten sich auf die Spezifikation der Klasse auswirken können.
- Das Open-Closed-Prinzip besagt, dass Software-Entitäten (Klassen, Module, Funktionen usw.) zur Erweiterung geöffnet, aber zur Änderung geschlossen sein sollten.
- Das Liskov-Substitutionsprinzip - Objekte in einem Programm sollten durch Instanzen ihrer Subtypen ersetzt werden können, ohne die Richtigkeit dieses Programms zu ändern.
- Das Prinzip der Schnittstellentrennung besagt, dass kein Client gezwungen werden sollte, sich auf Methoden zu verlassen, die er nicht verwendet.
- Das Abhängigkeitsinversionsprinzip - ist eine spezielle Form der Entkopplung von Softwaremodulen. Wenn Sie diesem Prinzip folgen, werden die herkömmlichen Abhängigkeitsbeziehungen, die von Richtlinienmodulen auf hoher Ebene zu Abhängigkeitsmodulen auf niedriger Ebene hergestellt werden, umgekehrt, wodurch Module auf hoher Ebene unabhängig von den Details der Modulimplementierung auf niedriger Ebene werden.
Aktivieren Sie eine hohe Warnstufe. Ignorieren Sie Warnungen nicht
Beim Kompilieren gibt es zwei Warnungen, und beide Warnungen weisen auf mögliche logische Probleme im Code hin.
Eine Warnung ist ein möglicher Datenverlust in dieser Zeile:
return middleBoard; // To return the character of the player who won
in Board::checkwin(). Diese Warnung ist darauf zurückzuführen, dass der Code eine size_tals a deklarierte Variable zurückgibt char.
Bei der zweiten Warnung handelt es sich ebenfalls um Board::checkwin()die Warnung, not all control paths return a valuedie in der letzten Zeile der Funktion ausgegeben wird. Dies ist möglicherweise die schwerwiegendere der 2 Warnungen, da sie definitiv auf mögliche logische Probleme im Code hinweist.
Bevorzugen Sie Casts im C ++ - Stil gegenüber Casts im alten C-Stil
Die folgende Codezeile verwendet eine alte C-Besetzung:
board[initialNum] == (char)Players::PLAYER_X ? scoreX++ : scoreO++;
C ++ hat seine eigenen Casts, die bessere Warnungen und Compilerfehler liefern. Dies sind static castsund dynamic casts. Statische Casts treten zur Kompilierungszeit auf und bieten mögliche Fehler oder Warnungen, wenn der Cast nicht typsicher ist. In der obigen Codezeile ist eine statische Umwandlung besser geeignet.
board[initialNum] == (static_cast<char>(Players::PLAYER_X)) ? scoreX++ : scoreO++;
Bevorzugen Sie selbstdokumentierenden Code gegenüber Kommentaren
Der Code enthält zu viele Kommentare. Eines der Dinge, die neuen Programmierern nicht bewusst sind, ist die Wartung von Code. Der von Ihnen geschriebene Code wird möglicherweise 20 oder mehr Jahre lang verwendet, und es ist durchaus möglich, dass Sie nicht so lange für das Unternehmen arbeiten. Wenn der Code viele Kommentare enthält, müssen die Kommentare sowie der Code selbst beibehalten werden. Dies kann den Arbeitsaufwand verdoppeln. Es ist besser, selbstdokumentierenden Code mit eindeutigen Variablen-, Klassen- und Funktionsnamen zu schreiben. Verwenden Sie Kommentare für Entwurfsentscheidungen oder Abstraktionen auf hoher Ebene. Wenn eine Funktion einen speziellen Ablaufzustand erfordert, wird dieser in einem Kommentarblock vor der Funktion angezeigt.
TROCKENER Code
Es gibt ein Programmierprinzip namens Don't Repeat Yourself-Prinzip, das manchmal als DRY-Code bezeichnet wird. Wenn Sie feststellen, dass Sie denselben Code mehrmals wiederholen, ist es besser, ihn in eine Funktion zu kapseln. Wenn es möglich ist, den Code zu durchlaufen, kann dies auch die Wiederholung verringern. Die Funktion Board::checkWin()enthält redundanten Code in den 3 Schleifen, die nach Gewinnen suchen. Es gibt mehrere Möglichkeiten, dies zu beheben, und in einer anderen Antwort wurde eine gute vorgeschlagen.
Komplexität
Die Funktion Board::checkWin()ist zu komplex (macht zu viel). Anstatt ein Zeichen zurückzugeben, Board::checkWin()sollte ein boolescher Wert zurückgegeben werden, der angibt, ob es sich um einen Gewinn handelt oder nicht. Andere Funktionen sollten die Aktualisierung der Karte mit den richtigen Zeichen implementieren. Die Komplexität dieser Funktion hat zur Warnung geführt not all control paths return a value.
Magische Zahlen
Die Board::checkWin()Funktion in jeder Schleife enthält magische Zahlen , die prüfen, ob ein Gewinn vorliegt. Es ist möglicherweise besser, symbolische Konstanten für sie zu erstellen, um den Code lesbarer und leichter zu pflegen. Diese Nummern können an vielen Stellen verwendet werden, und die Möglichkeit, sie durch Bearbeiten nur einer Zeile zu ändern, erleichtert die Wartung.
Numerische Konstanten im Code werden manchmal als magische Zahlen bezeichnet , da sie keine offensichtliche Bedeutung haben. Es gibt eine Diskussion darüber zum Stapelüberlauf .