0

I have recently coded Connect 4 in C++. I would like to know if there are any improvements that could be made to the code.

connect4.cpp:

#include <iostream>
#include <stdlib.h>

char grid[6][7];

void setup_grid();
void display_grid();
void player_turn(char);
bool four_in_a_row(char);
bool grid_is_full();

int main()
{
    char player = '1';

    setup_grid();
    system("cls");

    while(true)
    {       
        display_grid();
        player_turn(player);

        if(four_in_a_row(player))
        {
            system("cls");
            display_grid();
            std::cout << "Player " << player << " has won!" << std::endl;
            goto end;
        }
        else if(grid_is_full())
        {
            system("cls");
            display_grid();
            std::cout << "It is a draw." << std::endl;
            goto end;
        }

        if(player == '1')
            player = '2';
        else if(player == '2')
            player = '1';

        system("cls");
    }

end:
    std::cin.get();
    std::cin.get();

    return 0;
}

void setup_grid()
{
    for(int i = 0; i < 6; i++)
        for(int j = 0; j < 7; j++)
            grid[i][j] = '0';
}

void display_grid()
{
    std::cout << std::endl;
    std::cout << "Enter a column number (1-7) to put a piece into that column." << std::endl;
    std::cout << std::endl;

    for(int i = 0; i < 6; i++)
    {
        for(int j = 0; j < 7; j++)
            std::cout << grid[i][j] << ' ';

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

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

void player_turn(char player)
{
    int column_number;
    bool valid;

    do
    {
        valid = true;
        std::cout << "Player " << player << ": ";

        std::cin >> column_number;

        column_number--;

        if((column_number < 0) || (column_number > 6))
        {
            std::cout << "That number was not between 1 and 7" << std::endl;
            valid = false;
            continue;
        }

        if(grid[0][column_number] != '0')
        {
            std::cout << "Column is full." << std::endl;
            valid = false;
        }
    }
    while(!valid);

    for(int i = 0; i < 6; i++)
    {
        if(grid[i + 1][column_number] != '0')
        {
            grid[i][column_number] = player;
            break;
        }
        else if(i == 6)
        {
            grid[i][column_number] = player;
            break;
        }
    }
}

bool four_in_a_row(char player)
{
    // Horizontal check:

    for(int i = 0; i < 6; i++)

        for(int j = 0; j < 4; j++)

            if(grid[i][j] == player && grid[i][j+1] == player)
                if(grid[i][j+2] == player && grid[i][j+3] == player)
                    return true;

    // Vertical check:

    for(int i = 0; i < 3; i++)

        for(int j = 0; j < 7; j++)

            if(grid[i][j] == player && grid[i+1][j] == player)
                if(grid[i+2][j] == player && grid[i+3][j] == player)
                    return true;

    // Diagonal check:

    for(int y = 0; y < 3; y++)
    {
        for(int x = 0; x < 7; x++)
        {
            if(grid[y][x] == player)
            {

                // Diagonally left:
                if(grid[y+1][x-1] == player)
                {
                    if(grid[y+2][x-2] == player)
                        if(grid[y+3][x-3] == player)
                            return true;
                }

                // Diagonally right: (There is an error here)
                if(grid[y+1][x+1] == player)
                {
                    if(grid[y+2][x+2] == player)
                        if(grid[y+3][x+3] == player)
                            return true;
                }
            }
        }
    }

    return false;
}

bool grid_is_full()
{
    for(int y = 0; y < 6; y++)
    {
        for(int x = 0; x < 7; x++)
        {
            if(grid[y][x] == '0')
                return false;
        }
    }

    return true;
}

I know the goto statement (line 36) is bad programming practice but it seemed appropriate to the situation.

Should I use camelCase or snake_case?

On lines 48 and 49, why do I have to type std::cin.get() twice to get one input?

  • "I know the goto statement (line 36) is bad programming practice but it seemed appropriate to the situation." Wrapping the loop in a function and returning early on a win or a full grid would be more palatable. Naming scheme is a personal decision or defined by the organization's coding standard. Here it is off topic: Opinion based. "I have to type std::cin.get() twice to get one input?" Odds are you leave a newline from pressing enter in the stream. Easy to check by printing out what character you got. – user4581301 Jul 01 '18 at 19:47
  • 1) I'd use `break` instead of `goto` in this case. 2) This code looks very "C"--since you're in C++, consider using classes, vectors, iterators, lambdas and so on. 3) Your "interface" logic (`cout`) is coupled to the game logic, so your functions have side effects preventing reuse. Also, maybe this should be in CodeReview SE? – ggorlen Jul 01 '18 at 19:47
  • Stack overflow is for specific programming problems. When you ant to improve working code, you should ask at [Code Review](https://codereview.stackexchange.com/help). Note I've linked you to the help page. Read the Asking section and tune the question appropriately before posting a question there. – user4581301 Jul 01 '18 at 19:49
  • 1
    Next time you have a question like this, you might get even better results on CodeReview.SX. Glad you’re getting some answers here, though. – Davislor Jul 01 '18 at 20:45
  • It looks like a bug: `for(int x = 0; x < 7; x++)` x is up to 6. The "Diagonal Left" check: The code refers to `grid[y+1][x-1]` down to `[x-3]` but what if x = 0 ? Same the "Diagonal Right" check: The code refers to `grid[y+1][x+1]`, up to `[x+3]` but what if x>4? You have only 0-6 index values on grid x-axis. – Amit G. Jul 01 '18 at 23:16

1 Answers1

0

1) Put the I/J size in a #define instead of multiple use of 6 & 7.

2) Every time when a player place an entry reduce 1 from 6*7 count variable. When count variable reaches 0 grid_is_full.

Amit G.
  • 2,546
  • 2
  • 22
  • 30
  • 1
    Recommend `constexpr` in place of the `#define` if C++11 or better is available. Most compilers will do just as well with `const` without the messiness of macro substitution from `#define`. Whatever you use, the goal is to replace [the magic numbers](https://stackoverflow.com/questions/47882/what-is-a-magic-number-and-why-is-it-bad) with descriptive identifiers. – user4581301 Jul 01 '18 at 20:19