7
\$\begingroup\$

I just wrote a very simple game - "Connect 4". It's working -- at least I think so -- but I wanted to ask for some advice for the future and find out what can be done better. I'm really new to programming, but I want to learn coding (my mission - work on it 1 hour a day for one year).

#include <iostream>
#include <cstdlib>
#include <unistd.h>

int tab[7][6];
int choice, player;
bool end = false;
int a=0;

void check(int x)
{
    if(tab[x-1][a]!=0 && a<6)
    {
        a++;
        check(x);
    }   else if (player==1 && a<6)
            {
            tab[x-1][a]=1;
            a=0;
            }
        else if (player==2 && a<6)
            {
            tab[x-1][a]=2;
            a=0;
            }
        else
        {
        std::cout << "WRONG!" << std::endl;
        a=0;
        player++;
        }
}

int draw()
{
    system("clear");
    for(int i = 0; i<9; i++)
    {
        if(i<2)
        {
        std::cout<<"-";
        } else if(i>7)
        {
        std::cout<<i-1<<"--"<<std::endl;
        } else {
        std::cout<<i-1<<"----";
        }
    }
    for(int i = 0; i<6; i++)
    {
        for(int j = 0; j<7; j++)
        {
            if(tab[j][i]!=0)
            {
                if(tab[j][i]==1)
                {
                std::cout<<"| X |";
                }else std::cout<<"| O |";
            }
            else std::cout<<"|   |";
        } std::cout<<std::endl;
    }
    for(int i = 0; i<35; i++)
    {
        std::cout<<"=";
    } std::cout<<std::endl;
}

int win_check()
{
    for(int i = 0; i<6; i++)
    {
        for(int j = 0; j<7; j++)
        {
        if(tab[j][i]==1 && tab[j+1][i+1]==1 && tab[j+2][i+2]==1 && tab[j+3][i+3]==1)
            {
            end = true;
            std::cout << "\nPLAYER 1 WIN!" << std::endl;
            }
        if(tab[j][i]==1 && tab[j+1][i-1]==1 && tab[j+2][i-2]==1 && tab[j+3][i-3]==1)
            {
            std::cout << "\nPLAYER 1 WIN!" << std::endl;
            end = true;
            }
        if(tab[j][i]==2 && tab[j+1][i-1]==2 && tab[j+2][i-2]==2 && tab[j+3][i-3]==2)
            {
            std::cout << "\nPLAYER 2 WIN!" << std::endl;
            end=true;
            }
        else if(tab[j][i]==2 && tab[j-1][i-1]==2 && tab[j-2][i-2]==2 && tab[j-3][i-3]==2)
            {
            end = true;
            std::cout << "\nPLAYER 2 WIN!" << std::endl;
            }
        else if(tab[j][i]==1 && tab[j][i-1]==1 && tab[j][i-2]==1 && tab[j][i-3]==1)
            {
            std::cout << "\nPLAYER 1 WIN!" << std::endl;
            end = true;
            }
        else if(tab[j][i]==1 && tab[j-1][i]==1 && tab[j-2][i]==1 && tab[j-3][i]==1)
            {
            std::cout << "\nPLAYER 1 WIN!" << std::endl;
            end = true;
            }
        else if(tab[j][i]==2 && tab[j][i-1]==2 && tab[j][i-2]==2 && tab[j][i-3]==2)
            {
            std::cout << "\nPLAYER 2 WIN!" << std::endl;
            end = true;
            }
        else if(tab[j][i]==2 && tab[j-1][i]==2 && tab[j-2][i]==2 && tab[j-3][i]==2)
            {
            std::cout << "\nPLAYER 2 WIN!" << std::endl;
            end = true;
            }
        }
    }
}
int p_choice()
{
    player = 1;
    while(end!=true)
    {
    std::cout << "PLAYER " << player << ": ";
    std::cin >> choice;
        if (choice>0 && choice<8)
        {
            check(choice);
            draw();
            if (player == 1)
            {
                player++;
            }
            else
            {
                player--;
            }
        }
    else
        {
    std::cout << "WRONG CHOICE!" << std::endl;
        }
    win_check();
    }
}


int main()
{
    system("clear");
    std::cout<<"WELCOME IN CONNECT 4"<<std::endl;
    sleep(1);
    draw();
    p_choice();
    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This cannot possibly work correctly. win_check() indexes out of bounds. Step through your loop manually, and see what indices you compute in each iteration. \$\endgroup\$ – Cris Luengo Jan 11 '18 at 2:48
  • \$\begingroup\$ Looks cryptic. Some comments would have helped, and the indentation is weird. \$\endgroup\$ – Raimund Krämer Jan 11 '18 at 9:15
  • 4
    \$\begingroup\$ This question is being discussed on Meta. \$\endgroup\$ – 200_success Jan 12 '18 at 17:27
  • \$\begingroup\$ Thanks for reopening, hopefully we didn't scare off @included \$\endgroup\$ – Snowbody Jan 12 '18 at 22:41
  • 1
    \$\begingroup\$ @Snowbody: it doesn't write out of bounds, only reads. And the array is stack-allocated, so it's unlikely to show during runtime that this is happening. I've had plenty of errors like this when I was starting out. I'm glad I could help the OP learn about that, and a few other things. Thanks for pushing to reopen this question! \$\endgroup\$ – Cris Luengo Jan 12 '18 at 22:49
8
\$\begingroup\$

This is an excellent start for a starting programmer.

There are many things to comment on, of course. I'm going to focus on what I think is most important for a new programmer to work on.

Compiler warnings

When I compile your code, I get three warnings for functions that are non-void, but don't have a return statement. draw(), win_check() and p_choice() should all probably have been declared as void, rather than int.

Do make sure that you turn on all compiler warnings, the more picky the compiler gets the better, and pay attention to each warning that you get. Fix your code so you don't get any warnings. This is likely going to help you prevent a lot of errors.

As I mentioned in the comment above, win_check() is incorrect. I'll leave it as an exercise to the reader to figure out why and how to fix it.

Names

You do a good job overall with variable names and function names. I would choose to be even more verbose, use table instead of tab, and draw_table() instead of draw(). The exception here is a. Being a global variable, you want to make its meaning clear through its name. I have no idea what it does or means.

That said, it's not really a global variable, you only use it inside the check function, and you don't need to store it in between function calls, so why not make it a local variable?

Global variables

That said, it would be even better to avoid the use of global variables all together. For example, draw() could be declared as

void draw_table(int const table[7][6]);

and similarly for other functions, with main() "owning" all those variables and passing them along in the function calls. Note that passing an array as above to a function passes a pointer to the array, so the function can modify the table. I added const to the function declaration to indicate that draw_table() doesn't modify the table. Other functions that do modify it would take a non-const array as input.

Constants

You have values 6 and 7 scattered throughout your code. Make these into actual constants at the top of your file:

constexpr int table_width = 7;
constexpr int table_height = 6;

This has two advantages: (1) When you use for(int i=0; i<6; i++) in your code, it's not directly clear what the 6 is, if you use for(int i=0; i<table_height; i++) than it's immediately clear what you're doing. (2) If you decide that you want a different size table, it'll be easy to change. Using constants like this will reduce bugs significantly.

The one responsibility rule

All your functions, except for draw(), do multiple things. Try to separate writing to screen and the game logic into different functions. For example, win_check() should only check if there are four same-valued elements on a row. It would return true if someone won (doesn't change any global variables, doesn't write to screen). Note that only the current player can win, there is no move s/he can make that causes the other player to win. In your loop in p_choice() you then call win_check(), and act upon its return value:

void p_choice() {
  int table[table_width][table_height]
  player = 1;
  while(true) {
    std::cout << "PLAYER " << player << ": ";
    std::cin >> choice;
    if (choice>0 && choice<8) {
      check(table, choice);
      if (win_check(table)) {
        std::cout << "\nPLAYER " << player << " WINS!\n";
        return;
      }
      draw(table);
      player = player == 1 ? 2 : 1;
    } else {
      std::cout << "WRONG CHOICE!\n";
    }
  }
}

Style consistency

Please make sure that your indents are consistent, that the location of braces is consistent, etc. That makes the code so much easier to read, and therefore you'll make fewer bugs, and you'll have an easier time finding the bugs that you do make.

Looking forward to your next project!

\$\endgroup\$
  • \$\begingroup\$ He should also be using a std array, not a C array. Also the std endl can be replaced with a new line character. \$\endgroup\$ – JNS Jan 11 '18 at 10:40
  • \$\begingroup\$ @JNS Yes, and a ton of other things (encapsulation, data hiding, data ownership, ...). But you have to start somewhere. The std lib is pretty daunting when you're starting out. Learning to program well is more important that learning to use the std lib. Once you're writing good code, you can improve and simplify by learning to use the std lib appropriately. At least that's how I think about it. \$\endgroup\$ – Cris Luengo Jan 11 '18 at 14:22
8
\$\begingroup\$

Assuming you'll fix your bug.

Let's analyze your code.

#include <iostream>
#include <cstdlib>
#include <unistd.h>

I notice you've tagged your question with the "linux" tag, and you also use #include <unistd.h>. So I guess you already know that you've tied your code tightly to linux and porting it will be difficult. That's your call. But it's not hard to make a few changes to the code so it could be cross-platform. It involves wrapping the platform-specific functions (the system() and sleep() calls) in a function, and then having #ifdef/#endif pairs in the body of the function that chooses the appropriate code for the platform you're on.

Even if you stay on Linux, tying yourself to system calls is a bad idea. Consider what happens if you want to modify your code so it's not running on a terminal. Maybe you want to make it run as a web app, or a GUI using Qt or Tk toolkit. Sprinkling your code with calls to cout and system() makes this almost impossible. It's much better to separate the program's logic from user interaction, so that the same logic can be used easily in a different context.

int tab[7][6];
int choice, player;
bool end = false;
int a=0;

A few problems here. You're using global variables, which means the logic will be hard to incorporate elsewhere -- the variables might clash with other code that you integrate. At the very least, use C++ classes to encapsulate your data, so that it's only available where it needs to be. If not, declare the variables in the appropriate scope and pass them as parameters to functions that need them.

Also, you need to name these variables better. tab would be better if it were game_board or similar. player should be current_player, and I have no idea what a is supposed to be. Variable names need to be descriptive.

void check(int x)

Check? Check what? And what does x even mean here? What does this function even do? It's void so it obviously changes the global state, but its name conflicts with that.

{
    if(tab[x-1][a]!=0 && a<6)

These checks are out of order. If a>=6 then the first array reference will be out of bounds. You need to check a first.

    {
        a++;
        check(x);

This function does not need to be recursive. Recursion is unnecessary in almost all cases. Just use a loop instead. Programming courses teach recursion to force students to think, not because it is a good technique.

    }   else if (player==1 && a<6)
            {
            tab[x-1][a]=1;
            a=0;
            }
        else if (player==2 && a<6)
            {
            tab[x-1][a]=2;
            a=0;
            }

Notice anything similar about these two if clauses here? They can easily be combined. Don't repeat yourself. The computer is good at repetition, let it do what it's good at. What about tab[x-1][a] = player?

        else
        {
        std::cout << "WRONG!" << std::endl;
        a=0;
        player++;
        }
}

So as I understand this, on an invalid move, the player get yelled at and loses their turn? That's harsh! People are imperfect beings, they make mistakes. Why make the computer program deliberately punitive? Work with your user, don't fight with them. Also you have a bug here, what if player 2 makes a mistake, then player becomes 3 and the program fails.

One final note: It's almost never necessary to use std::endl. The output buffer will automatically be flushed at the right time, e.g. when the user needs to provide input. Invoking std::endl yourself will just degrade performance. Just stick to "\n".

int draw()
{
    system("clear");
    for(int i = 0; i<9; i++)
    {
        if(i<2)
        {
        std::cout<<"-";
        } else if(i>7)
        {
        std::cout<<i-1<<"--"<<std::endl;
        } else {
        std::cout<<i-1<<"----";
        }
    }

Wait wait wait. Why do you loop over i then do different things depending on the value of i? That's not what a loop is for. Just make separate code! Loop over what's common, and avoid i-1 type constructions, they'll mess you up.

    for(int i = 0; i<6; i++)
    {
        for(int j = 0; j<7; j++)
        {
            if(tab[j][i]!=0)
            {
                if(tab[j][i]==1)
                {
                std::cout<<"| X |";
                }else std::cout<<"| O |";
            }
            else std::cout<<"|   |";
        } std::cout<<std::endl;

Look into the switch statement of C. Again, no need for std::endl.

    }
    for(int i = 0; i<35; i++)
    {
        std::cout<<"=";
    } std::cout<<std::endl;
}

int win_check()
{
    for(int i = 0; i<6; i++)
    {
        for(int j = 0; j<7; j++)
        {
        if(tab[j][i]==1 && tab[j+1][i+1]==1 && tab[j+2][i+2]==1 && tab[j+3][i+3]==1)
            {
            end = true;
            std::cout << "\nPLAYER 1 WIN!" << std::endl;
            }
        if(tab[j][i]==1 && tab[j+1][i-1]==1 && tab[j+2][i-2]==1 && tab[j+3][i-3]==1)
            {
            std::cout << "\nPLAYER 1 WIN!" << std::endl;
            end = true;
            }
        if(tab[j][i]==2 && tab[j+1][i-1]==2 && tab[j+2][i-2]==2 && tab[j+3][i-3]==2)
            {
            std::cout << "\nPLAYER 2 WIN!" << std::endl;
            end=true;
            }
        else if(tab[j][i]==2 && tab[j-1][i-1]==2 && tab[j-2][i-2]==2 && tab[j-3][i-3]==2)
            {
            end = true;
            std::cout << "\nPLAYER 2 WIN!" << std::endl;
            }
        else if(tab[j][i]==1 && tab[j][i-1]==1 && tab[j][i-2]==1 && tab[j][i-3]==1)
            {
            std::cout << "\nPLAYER 1 WIN!" << std::endl;
            end = true;
            }
        else if(tab[j][i]==1 && tab[j-1][i]==1 && tab[j-2][i]==1 && tab[j-3][i]==1)
            {
            std::cout << "\nPLAYER 1 WIN!" << std::endl;
            end = true;
            }
        else if(tab[j][i]==2 && tab[j][i-1]==2 && tab[j][i-2]==2 && tab[j][i-3]==2)
            {
            std::cout << "\nPLAYER 2 WIN!" << std::endl;
            end = true;
            }
        else if(tab[j][i]==2 && tab[j-1][i]==2 && tab[j-2][i]==2 && tab[j-3][i]==2)
            {
            std::cout << "\nPLAYER 2 WIN!" << std::endl;
            end = true;
            }
        }
    }
}

Again, a lot of repeated code here. See if you can figure out what's in common and reduce the amount of code here, using the computer's ability to do repeated operations easily by using loops.

int p_choice()
{
    player = 1;
    while(end!=true)

Since end is bool you can just use while (!end)

    {
    std::cout << "PLAYER " << player << ": ";
    std::cin >> choice;
        if (choice>0 && choice<8)
        {
            check(choice);
            draw();
            if (player == 1)
            {
                player++;
            }
            else
            {
                player--;
            }

What mathematical operation do you know that changes 1 to 2 and 2 to 1? (hint: think about subtraction)

        }
    else
        {
    std::cout << "WRONG CHOICE!" << std::endl;
        }
    win_check();
    }
}


int main()
{
    system("clear");
    std::cout<<"WELCOME IN CONNECT 4"<<std::endl;
    sleep(1);
    draw();
    p_choice();
    return 0;
}

For sleep() there is already crossplatform sleep() if you are using C++11 -- see this question. If you're not on C++11, you could use the Boost library that provides a crossplatform sleep(). But I question whether a delay is necessary at all. How does this delay improve the user experience? If the user launches your program, will they really want to wait before they're able to type an input?

\$\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.