2
\$\begingroup\$

This is my first time using StackExchange. I did not have any idea what this website for, but I guess is a place to review the code? So, I'm just going to give it a shot.

#include <iostream>

std::string board[3][3];

void making_board();
void play_the_game();
void display_board();
void choose_position(int position, std::string* current_player);
void find_row_column(int position, int* row, int* column);
void player_turn(std::string* current_player);
std::string computer_turn(std::string* current_player);
std::string predict_future(std::string* current_player);
std::string check_the_game();
int possible_move(int* row, int* column, std::string *current_player, std::string* position);
int move_left(int row, int column, std::string* current_player);
int predict_opponent(int* row, int* col, std::string opponent, std::string* ori_position);
int possible_opponent(int* row, int* column, std::string opponent);

int main()
{
    play_the_game();
    return 0;
}

std::string check_the_game(){
    int area_filled = 0;
    for (int row = 0; row < 3; row++){
        if (board[row][0] == board[row][1] && board[row][1] == board[row][2]){
            return board[row][0];
        }
    }
    for (int column = 0; column < 3; column++){
        if (board[0][column] == board[1][column] && board[1][column] == board[2][column]){
            return board[0][column];
        }
    }
    if (board[0][0] == board[1][1] && board[1][1] == board[2][2])return board[1][1];
    else if (board[2][0] == board[1][1] && board[1][1] == board [0][2]) return board[1][1];
    for (int rows = 0; rows < 3; rows++){
        for (int columns = 0; columns < 3; columns++){
            if (board[rows][columns] == "X" || board[rows][columns] == "O")
            area_filled++;
            if (area_filled == 9)return "neither";
        }
    }
    return "notyet";
}

int possible_opponent(int row, int column, std::string opponent){
    std::string ori_position = board[row][column];
    board[row][column] = opponent;
    if (check_the_game() == "X"){
        board[row][column] = ori_position;
        return -10;
    }else {
        board[row][column] = ori_position;
        return 0;
    }
}

int predict_opponent(int* row, int* col, std::string opponent, std::string* ori_position){
    int score;
    for (int r = 0; r < 3; r++){
        for (int c = 0; c < 3; c++){
            if (board[r][c] != "X" && board[r][c] != "O"){
                if (possible_opponent(r, c, opponent) == -10){
                    board[*row][*col] = *ori_position;
                    *row = r;
                    *col = c;
                    return -10;
                } else continue;
            }else continue;
        }
    }
    return 0;
}

int possible_move(int* row, int* column, std::string *current_player, std::string* position){
    std::string ori_position = board[*row][*column];
    board[*row][*column] = *current_player;
    if (check_the_game() == "O"){
        board[*row][*column] = ori_position;
        return 10;
    }else if (predict_opponent(row, column, "X", &ori_position) == -10){
        *position = board[*row][*column];
        board[*row][*column] = "O";
        return -10;
    }else {
        board[*row][*column] = ori_position;
        return 0;
        }
}

std::string predict_future(std::string* current_player){
    std::string position;
    int score;
    for (int row = 0; row < 3; row++){
        for (int column = 0; column < 3; column++){
            if (board[row][column] != "X" && board[row][column] != "O"){
                if (possible_move(&row, &column, current_player, &position) == -10){
                    return position;
                }
                else if (possible_move(&row, &column, current_player, &position) == 10){
                    position = board[row][column];
                    board[row][column] = *current_player;
                    return position;
                }else if (move_left(row, column, current_player) == 1){
                    position = board[row][column];
                    board[row][column] = *current_player;
                    return position;
                }else if(possible_move(&row, &column, current_player, &position) == 0){
                    if (board[1][1] != "X" && board[1][1] != "O" && move_left(row, column, current_player) < 9){
                        position = board[1][1];
                        board[1][1] = *current_player;
                        return position;
                    }else continue;
                }
            }else continue;
        }
    }
}

int move_left(int row, int column, std::string* current_player){
    int move_left = 0;
    for (int r = row; r < 3; r++){
        for (int c = column; c < 3; c++){
            if (board[r][c] != "X" && board[r][c] != "O"){
                move_left++;
            }
        }
    }
    return move_left;
}

void play_the_game(){
    bool still_playing = true;
    std::string current_player = "X";
    std::string winner = "";
    making_board();
    while (still_playing){
        display_board();
        player_turn(&current_player);
        winner = check_the_game();
        if (winner == "X" || winner == "O" || winner == "neither"){display_board();break;}
    }
    std::cout << " Winner is " << winner << std::endl;
    std::cout << " Play again (1 for yes and 0 for no)? ";
    std::string answer;
    std::cin >> answer;
    if (answer == "1"){
        system("CLS");
        play_the_game();
    }else if(answer == "0"){
        exit(0);
    }else {
        std::cout << " Invalid answer";
        std::cout << " Play again (1 for yes and 0 for no)? ";
    }
}

void player_turn(std::string *current_player){
    std::string sposition;
    int position;
    if (*current_player == "O"){
        sposition = predict_future(current_player);
        std::cout << " " << *current_player << "'s turn, position chosen was: " << sposition << std::endl;
        *current_player = "X";
    }else {
        std::cout << " " << *current_player << "'s turn, choose position: ";
        std::cin >> position;
        if (!std::cin >> position){
            std::cin.clear();
            std::cin.ignore(1000, '\n');
            std::cout << " Position is not available" << std::endl;
            player_turn(current_player);
        }
        choose_position(position, current_player);
    }
}

void choose_position(int position, std::string *current_player){
    int row, column;
    find_row_column(position, &row, &column);
    if (board[row][column] == "X" || board[row][column] == "O" || position < 0 || position > 9){
        std::cout << " Position is not available" << std::endl;
        player_turn(current_player);
    } else {
        board[row][column] = *current_player;
        if (*current_player == "X") *current_player = "O";
        else *current_player ="X";
    }
}

void find_row_column(int position, int* row, int* column){
    *row = position/3;
    if (*row = 0) *column = 2;
    *column = position-*row-1;
}

void display_board(){
    std::cout << "\n";
    for (int rows = 2; rows >= 0; rows--){
        for (int columns = 0; columns < 3; columns++){
            if (columns == 0){
                std::cout << "  " << board[rows][columns];
            } else std::cout << " | " << board[rows][columns];
        }
        if (rows > 0)std::cout << "\n ---|---|---\n";
        else std::cout << "\n\n";
    }
}

void making_board(){
    int i = 1;
    for (int rows = 0; rows < 3; rows++){
        for (int columns = 0; columns < 3; columns++){
            board[rows][columns] = std::to_string(i);
            i++;
        }
    }
}

I tried to beat my AI. Somehow I still won.

\$\endgroup\$
1
  • \$\begingroup\$ Hi. Welcome to Code Review! Your post is rather chatty. We can already see that you are a first time user. It shows in your profile and your reputation score. We would prefer that you take the tour before posting. Then you wouldn't need to tell us you have no idea what the site does. And finally, it would help if you told us, in words, what your code does. Yes, it's Tic-Tac-Toe. Does it take user input? For how many players? Does it provide an AI? For how many players? \$\endgroup\$
    – mdfst13
    Sep 12, 2021 at 13:49

4 Answers 4

1
\$\begingroup\$

Code structure review

  1. It is C-style code. As you have added C++ tag, I assume you intend to write C++ code. Think from OOP point of view and abstractions will improve. Methods and signatures will improve and unnecessary passing of parameters will be avoided.
void choose_position(int position, std::string* current_player);
void find_row_column(int position, int* row, int* column);
void player_turn(std::string* current_player);
std::string computer_turn(std::string* current_player);
std::string predict_future(std::string* current_player);
std::string check_the_game();
int possible_move(int* row, int* column, std::string* current_player, std::string* position);
int move_left(int row, int column, std::string* current_player);
int predict_opponent(int* row, int* col, std::string opponent, std::string* ori_position);
int possible_opponent(int* row, int* column, std::string opponent);
  1. Board representation: Using 2D string array to represent board is not right choice. It is making code unnecessarily complex. Boolean array will be a better choice. For example function check_the_game is doing lots of string comparisons. With boolean array, it can be done using few AND operations. Return value of this function can be a int (or enum).

  2. Model and view should be separated. It will also allow you to bind any shape you want to use to represent icons for two player.

  3. Refs vs pointers: Code can be written with refs rather than pointers.

  4. Strings "X" and "O" is sprinkled throughout the code. It should be avoided.

  5. Random constants like -10, 10, 9, 1000 etc sprinkled throughout the code. It should be avoided.

Summary: Rethink from OOP perspective. Separate model and view. Avoid random constants.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ The board representation has at least three states: X played, O played, and free. So even without the free space numbering in the same data structure, an array of bool doesn't make sense. \$\endgroup\$
    – JDługosz
    Sep 14, 2021 at 21:08
  • \$\begingroup\$ @JDługosz: You are correct. Regular bool won't help as it can hold only two states. But we can use std optional<bool>. Or enum class will three values. \$\endgroup\$
    – nkvns
    Sep 15, 2021 at 4:21
  • \$\begingroup\$ @nkvns Thank you for the review, this really is a good review. I appreciate it and will take the advice and suggestion. I just struggle on how to think in OOP way. \$\endgroup\$ Sep 16, 2021 at 4:04
1
\$\begingroup\$

No time for a full review, but here are some general remarks:

  • making_board: try and use verbs, as methods are things that perform actions, i.e. make_board or setup_board (you seem to do this right for the other methods, so maybe this is just the odd one out).
  • C++ code often differs from C by using capital letters instead of underscores to separate method names. Style is subjective though - as long as you are aware of this.
  • Use explicit WIDTH and HEIGHT instead of literals valued 2, 3 and 9.
  • check_the_game should have separate methods to check for rows, columns and diagonals. You could also explicitly define "groups", which could be either, but this is a more advanced trick (I use it for my SudokuSolver code, where a Sudoku can have many forms).
  • You could use an enum with PLAYING, DRAW, WIN_O and WIN_X constants, try and avoid "stringly typed" code.
  • Even if you don't split your code in separate classes, it makes sense to order them somewhat, e.g. first main, then play_the_game etc.. The declarations and definitions should probably have the same order as well, so people don't have to search when using a text editor (or GitHub raw code, yuck).
  • Try and avoid readers to have to guess what e.g. -10 means, a constant value could have been used.
  • else continue is completely unnecessary at the end of a code block within a loop - trust me, any loop will continue automatically; it's what they do.
  • You could try and make a stream of positions instead of iterating over them all the time (this goes into lambda / functional territory though). This would also make sure that you don't get "X-mas tree code blocks" with many nested for and if statements.
  • Closing parentheses should be followed by a space, e.g. ) { instead of ){, maybe put your code though lint or another formatter / static style analyzer.
\$\endgroup\$
4
  • \$\begingroup\$ C++ code often differs from C by using capital letters instead of underscores to separate method names. Some programmers like capital letters... if you read the C++ standard and its library, though, as well as the original (1980s) books, you'll see that it's orthodox to use lowercase and underscores. And, C++ doesn't have "methods". Those are not even member functions of a class, so calling them methods is rather odd. (they are "free functions") \$\endgroup\$
    – JDługosz
    Sep 13, 2021 at 14:33
  • \$\begingroup\$ Many reviewers will complain about using ALL_CAPS for symbol names, reserving their use for macros. I'm sure you were not advocating using #define, right? \$\endgroup\$
    – JDługosz
    Sep 13, 2021 at 14:35
  • 1
    \$\begingroup\$ That's just my Java background leaking through :) I'll fix when I have the time. \$\endgroup\$ Sep 13, 2021 at 14:54
  • 1
    \$\begingroup\$ Good arguments in this review, but C++ code often differs from C by using capital letters instead of underscores to separate is certainly not true. Lot's of programmers prefer snake case, have a look at the STL. \$\endgroup\$
    – user228914
    Sep 14, 2021 at 6:45
1
\$\begingroup\$

Use another type for board representation

Maintaining an array of strings, constantly making comparisons and possibly copying it in a few places can get very expensive very fast, which makes representing the board internally using std::string inefficient.

Using a plain char is more suitable, maybe even Enums to map the characters to integral vales.

Use references

Prefer using references over pointers when possible. Aside from looking cleaner syntactically, using a reference makes your intention clear. This SO thread contains further discussion on this topic

More well-defined types

It seems to me that the code quality could largely benefit from defining proper types for a few things.

A very simple example from your code is the way you define positions on the board.

Even the ancient struct Point {int x, y; } with a few helper functions can make the code clearer, as you don't have to rely on passing around raw ints everywhere. The same applies to your board re-presentation. Have a look at object-oriented programming.

\$\endgroup\$
0
\$\begingroup\$
int possible_opponent(int row, int column, std::string opponent){

You're passing opponent by value, which duplicates the string. It's normal to pass strings by const reference, or better yet use a string_view.

    std::string ori_position = board[row][column];
    board[row][column] = opponent;
    if (check_the_game() == "X"){
        board[row][column] = ori_position;
        return -10;
    }else {
        board[row][column] = ori_position;
        return 0;
    }
}

So, you make a move, check the result, and then undo the move?
At the very least, it's poor to duplicate the board[row][column] = ori_position; in both branches.

If you make your board using byte-sized enumeration constants or characters rather than string objects for each cell, it becomes practical to copy the entire board state. In fact, a 9-byte board is smaller than a string instance! Making intermediate copies of the state is more normal for this kind of mini-maxi or speculative play algorithm.

\$\endgroup\$

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.