I need help about my code please :)

[King25]

Hello I'm working on Connect 4 project i have done everything so far but somehow i couldn't make the check functions can someone help me about it please :)

Here is my code:


Sorry i could not print the code here because its didn't fit here but i put the code to GitHub here is the link :)

https://gist.github.com/King252525/25d5c3d08e3c81340cee43ed30072435

[Niccolo]

@King25, For all those considering a response it was good that you included a way to get to the entire code, but I have to admit it is not easy to figure out what exactly you need help with.

There is one function named check, is that where you require help?

This is the check function from your code:

/*Win/Lose Check*/
bool check(int a, int b)
{
	int vertical = 1;
	int horizontal = 1;
	int diagonal1 = 1;
	int diagonal2 = 1;
	char player = place[a][b];
	/*Vertical*/
	int i;
	/*Horizontal*/
	int ii;

	/*Vertical Check (|)*/
	for (i = a + 1; place[i][b] == player && i <= 5; i++, vertical++);//Check Down
	for (i = a - 1; place[i][b] == player && i >= 0; i--, vertical++);//Check UP
	if (vertical >= 4)return true;

	/*Horizontal Check (-)*/
	for (ii = b - 1; place[a][ii] == player && ii >= 0; ii--, horizontal++);//Check Left
	for (ii = b + 1; place[a][ii] == player && ii <= 6; ii++, horizontal++);//Check Right
	if (horizontal >= 4) return true;

	/*Diagonal Check 1 (/)*/
	for (i = a - 1, ii = b - 1; place[i][ii] == player && i >= 0 && ii >= 0; diagonal1++, i--, ii--);//Up And Left
	for (i = a + 1, ii = b + 1; place[i][ii] == player && i <= 5 && ii <= 6; diagonal1++, i++, ii++);//Down And Left
	if (diagonal1 >= 4) return true;

	/*Diagonal Check 2 (\)*/
	for (i = a - 1, ii = b + 1; place[i][ii] == player && i >= 0 && ii <= 6; diagonal2++, i--, ii++);//Up And Right
	for (i = a + 1, ii = b - 1; place[i][ii] == player && i <= 5 && ii >= 0; diagonal2++, i++, ii--);//Up And Left
	if (diagonal2 >= 4) return true;

	return false;
}

The comment for "int drop(int)" indicates it' a "drop check", so perhaps you're including that as part of your request. It is:

/*Drop Check*/
int drop(int b)
{

	int count = 5;

	while (place[count][nowplace - 1] != 32 && count > -1) count--;

	if (count == -1) {
		return -1;
	}
	else {
		place[count][nowplace - 1] = player;
		return player;
	}

	//return 0;
}

Am I on the right track figuring out what you're asking?

Ok, not yet figuring out what the plan is, here (which appears to be central to your question, you're not sure how to get this working?)...

I can begin with a few points to consider.

This appears to be C for the most part, but you do include <string> and <iostream> (and use cout) which indicate you expect a C++ compiler. Is the adherence to "C style" of code design and usage intentional, and is it really required?

Though I don't usually adhere to Linus Torvalds (the curator of the Linux operating system, and the person for which it's "named"), he does have one "coding standard" offered as a quip, which I paraphrase here for uncertainty of the exact quote:

"If you have more than 3 nested brackets, you need to fix your code".

Torvalds works all but exclusively in C, and infamously berated C++ as a language in the 90's. Even in C++, though, take Torvald's point to heart. You have a great many nested brackets in your code, which is in the context of very long functions. It is a widely agreed upon coding standard that such code promotes bugs and destroys clarity. The "start_game" function is an example of this observation.

In check (above), I notice something which also strikes me as a style to avoid. I repeat an example here:

	for (i = a - 1, ii = b + 1; place[i][ii] == player && i >= 0 && ii <= 6; diagonal2++, i--, ii++);//Up And Right 

I'm not claiming this is wrong, but I can't really tell what the plan is here. It would seem that place is the 2D array representing the board.

What I'm talking about here is clarity in style. There are programmers coming to C and C++ from other backgrounds for which this style was, once, considered an advantage. A clearer implementation will be larger (more text), and some think that compacting tests and initialization like this is good work, but frankly it is confusing.

Many coding standards start out on the subject of for loops declaring (as an instruction to the programmer) to make the for loop itself simple. The initialization should be local (this is frequently optional), it should be simple (usually only that thing being looped, the index or count or whatever). The increment should also be kept simple (again limited to the index or count). The test should be only that which controls the loop, not more generalized logic which is the purpose of the loop.

Put another way, most coding standards do not accept for loops that have no bracketed code, but end in a ';'.

The reason is, among many other points, we can't quickly and immediately see what the loop is doing, separated from how it is controlled. In this style the two notions are mixed together, and we have to engage in more mental gymnastics just to read it that would be required if what the loop is doing is clearly isolated from how the loop is controlled.

I acknowledge that to those who prefer the style you're using here complain that it makes the code larger, but the overriding objective is clarity. We know from experience that clarity leads to fewer bugs, lets other members of a team understand the code, improves their productivity in working on the code, and all of that even applies to the original author when, much later, those assumed details have left memory and you will be reading the code as if you aren't the author (your future self).

Decades ago, as a child, I learned BASIC (the old, interpreted, line oriented BASIC typical of personal computers of the late 70's). When I learned C, my BASIC experience motivated me to write this way. I learned a few hard and rather illuminating lessons on this point. While it is fortunate for me to be a memory from 40 years ago, I will never forget when it really hit me like a board in the face. Just unwrapping this kind of construction into a clearer (though larger) version, which clearly separated process from sequencing, let me see what I could not see before. That alone let me fix a "bug" and a failure of logic I could not recognize otherwise, and it was immediate simply after reorganizing that code for clarity.

I'm not really sure what "check" is doing, or the theory of operation here. I suspect these loops should actually be functions...but then I'm not even really sure (beyond a shallow recognition) of the game itself.

I think the objective is for this function, check, to return true if 4 objects belonging to the player form a line. What I'm not really certain of, relative to both the game itself and the context of this function, if that is a test of the entire board every time, or if that test should begin with that player's most recent move.

I suspect efficiency may be found using that last phrase as a starting point, but I'm not sure yet.

We have to "talk" first, and foremost in that conversation is to inquire about whether or not you are willing to refactor this code into a C++ design, and why not if so.

[Niccolo]

I start a new reply because of limits, and because of the last point I made. I can't say I fully understand the game, but from what I recall I have reason to think you can test for a win at each play by starting with the last piece played, and end your test without checking the entire board.

For that to be true, these points have to be correct in my assumption about the game.

There are two players (could be more I suppose), and there are no moves a player could make which immediately gave ANOTHER player a win. One piece dropped could only result in a win for the player making that move, or not a win (moving to the next/other player).

Once a win is noted, the game is over.

I assert that if this (and possibly a few other caveats I've missed) is true, you can detect a win by testing at each play by merely considering only the last piece dropped, and what 'connections' it makes near it. No other arrangement on the "board" (I recall this is a vertical playing surface, viewable from both sides) could, at that moment, mark a win - though one piece could 'win' by more than one 'connection series' of 4 pieces.

I propose you consider a kind of primitive 'ray cast' approach, centered from the last piece played. Ray casting is used in 2D and 3D games to see if something is hit, or skewered. In this case it is 'primitive' (or maybe I should say simplified) by the 2D grid nature of the board. There are, therefore, 8 possible directions around the target piece (the last one played) upon which a 4 way connection may be discovered.

I believe there's no need to look upward. The way the piece is played, the upward direction would only find either 'beyond the edge' of the board, or empty places. This quickly eliminates 1 of the 8 possible tests.

Proceeding clockwise, the next direction to consider is a direction expressed by the simple 2D vector (1,-1). If, for example, the target piece landed at 4,3, the next potential connection in this direction (45 degrees, upper right direction) is at the target location plus this direction vector: (4,3) + (1,-1) = (5,2). If (5,2) does not contain the player's piece, the test for this direction ends without finding a match.

The next vectors to consider are, in order moving clockwise from this first test, (1,0) (90 degrees), (1,1) (45 degrees, lower right), (0, 1) (down), (-1,1) (down left), (-1,0) (left), and (-1,-1) (upper left). These vectors allow simple addition to "move" in the direction indicated. All series of connections of 4 must be in one (or more) of these directions from the last piece played.

So, let's say we've proceed to vector (1,1), the lower right direction, starting at (4,3). This direction implies that we'll be examining places for (5,4), (6,5) and (7,6). There's no row 7. The board is defined as a 6 by 7 2D array. There can be no match in this direction from this play. The maximum extent would be (5,4), because 5 is the last row (the 6th row starting from 0 in that dimension). However, we can also subtract vectors. If we find that (5,4) is a match, we should then "look backwards" from (4,3) to finish the test.

This implies we have now counted 2 matches, and we then calculated from (4,3), subtracting the current vector (1,1), and thus examine (3,2). If THAT is a match, we then take the position (3,2), subtract (1,1), resulting in position (2,1). If THAT is a match, we have now counted 4 are connected. This is the win.

Take that walk through for the 7 valid directions to consider (as 'UP' may never have a match), and you have a more concise plan that should be a BIT easier to code reliably.

[dhayden]

There are two key functions to playing the game: drop() and check(). All the rest is just user interface.

Your check logic looks good, except that you should do the bounds checks before accessing he place array. Ie, change this:
for (i = a + 1; place[i][b] == player && i <= 5; i++, vertical++);//Check Down
to this:
for (i = a + 1; i <= 5 && place[i][b] == player; i++, vertical++);//Check Down

        /*If Draw*/
        if (place[6][7] == 42) {

Both indexes are out of bounds. Remember, legal indexes start from 0. Also, shouldn't you be checking charsPlaced instead of place[6][7]?

Some general comments:
comments like /*Declaration*/ and /* Void Stuff */ don't add anything. The code itself indicates that these are declarations or void functions. Comments on the code should say why and how you're doing things. Comments on data should say what the data is used for and how your data encodes it. Comments on functions should say what it does, what the parameters mean and what the return value means.

Don't use magic constants. For example, the dimensions of the board should be constexprs (or const ints if you're using an old compiler). Changing the size of the board should be a one-line change.

Same thing for 15 and 254 (the values indicating the players 1 and 2) and lots of values inside display()

One function should do one thing. display() should not switch players.

drop() has an parameter (b) but doesn't use it.

There appears to be tons of duplicate code when one user wins.

[King25]

Dear @Niccolo and @dhayden ;
First of all i want to say im very beginner C++ learner :)
If u try to run the game u will see checks are not working properly.
I have done everything as far i know to make this game run but i'm stuck in here could you please tell me the right codes so then i can fix it :) because my English is very bad and i can understand a very less so please help me to fix this game and this is my final project and it will effect my 60% mark :)

Best Regards;
King25

[dhayden]

Your terminal control code wasn't working for me under cygwin so I cut the program down the minimum and got it working. You can use this as a guide.

Some things I noticed:
- variable names like row and column to make it clearer what things mean. See my code for drop() as an example.
- I don't think I had to change your check() code at all.
- I did change some code in start_game().

I hope this helps.

#include <iostream>
//#include "graphics.h"
#include <Windows.h>
#include <string>
#include <thread>
#include <time.h>
#include "conio.h"
// #include <system.h>

using namespace std;

//#pragma comment(lib, "graphics.lib")
//#pragma comment(lib, "winmm.lib")

/*Bool(Keep Looping)*/
bool check(int a, int b);
bool isKeyPressed(char character);

/*Declaraction*/
int drop(int b, char player);
int nowplace = 1;
const int ROWS=6;
const int COLS=7;
char place[ROWS][COLS] = { 0 };

char player = '1';

/*Void Stuff*/
void gotoxy(short x, short y);
void print_string(string mystring);
void print_string2(string mystring);
void print_string3(string mystring);
void textcolor(int x);
void resizeConsole(int width, int height);
void print_box(int wid, int hei, int x, int y);
void keyimput();
void play_music();
void start_game();
void display();
void welcome_screen();
void menu();
void username();
void how_to_play();
void exit_game();
void score_board();
void credits();
void music_menu();


// Get location to place the next piece
int getDropLocation()
{
    int result;
    while (true) {
	cout << "Enter column to drop into: ";
	cin >> result;
	if (result >0 && result <= COLS) {
	    return result-1;	// convert from user col to computer col
	}
    }
}

/*Starts Game*/
string user_name1, user_name;
void
start_game()
{
    for (int a = 0; a < ROWS; a++) {
	for (int b = 0; b < COLS; b++)
	    place[a][b] = ' ';
    }
    player = '1';

    display();

    /*Declaring Veriables */
    int col, row;
    int charsPlaced = 0;
    bool gamewon = false;
    while (true) {
	col = getDropLocation();
	row = drop(col, player);
	display();

	/*Error If Row Is FULL */
	if (row == -1)
	    cout << "Column is full\n Choose different place.";
	else {
	    charsPlaced++;
	    /*Check If Game Is Running */
	    gamewon = check(row,col);
	    if (gamewon || charsPlaced == ROWS*COLS) {
		break;
	    }
	    player = (player == '1' ? '2' : '1');
	    /*Display Lastest Board */
	}
    }
    
    // You can win on the last move, so check for game won
    // before checking for
    if (gamewon) {
	/*If Won By Player 2 */
	if (player == '1') {
	    cout << "player 1 wins\n";
	}
	else {
	    cout << "player 2 wins\n";
	}
    } else if (charsPlaced == ROWS*COLS) {
	cout << "No winner, Game was draw\n";
	system("pause");
    } else {
	// Why am I here?
	cout << "Came over but nobody won and the board isn't empty.\n";
    }

    system("pause");
}

/*Prints The Game Shape*/
void
display()
{
    //display header
    for (int i=0; i<COLS; ++i) {
	cout << ' ' << i+1 << ' ';
    }
    cout << "\n\n";

    //display board
    for (int a = 0; a <= 5; a++) {
	for (int b = 0; b <= 6; b++) {
	    cout << ' ' << place[a][b] << ' ';
	}
	cout << '\n';
    }
}

/*Win/Lose Check*/
bool
check(int a, int b)
{
    int vertical = 1;
    int horizontal = 1;
    int diagonal1 = 1;
    int diagonal2 = 1;
    char player = place[a][b];
    /*Vertical */
    int i;
    /*Horizontal */
    int ii;

    cout << "Checking " << place[a][b] << " at " << a << ' ' << b << '\n';
    /*Vertical Check (|) */
    for (i = a + 1; place[i][b] == player && i <= 5; i++, vertical++);	//Check Down
    for (i = a - 1; place[i][b] == player && i >= 0; i--, vertical++);	//Check UP
    if (vertical >= 4)
	return true;

    /*Horizontal Check (-) */
    for (ii = b - 1; place[a][ii] == player && ii >= 0; ii--, horizontal++);	//Check Left
    for (ii = b + 1; place[a][ii] == player && ii <= 6; ii++, horizontal++);	//Check Right
    if (horizontal >= 4)
	return true;

    /*Diagonal Check 1 (/) */
    for (i = a - 1, ii = b - 1; place[i][ii] == player && i >= 0 && ii >= 0; diagonal1++, i--, ii--);	//Up And Left
    for (i = a + 1, ii = b + 1; place[i][ii] == player && i <= 5 && ii <= 6; diagonal1++, i++, ii++);	//Down And Left
    if (diagonal1 >= 4)
	return true;

    /*Diagonal Check 2 (\) */
    for (i = a - 1, ii = b + 1; place[i][ii] == player && i >= 0 && ii <= 6; diagonal2++, i--, ii++);	//Up And Right
    for (i = a + 1, ii = b - 1; place[i][ii] == player && i <= 5 && ii >= 0; diagonal2++, i++, ii--);	//Up And Left
    if (diagonal2 >= 4)
	return true;

    return false;
}

/* drop a piece for the "player" into column "col". Returns the row
   where it was dropped, or -1 if the column is full*/
int
drop(int col, char player)
{
    int row;
    for (row = 0; row < ROWS; ++row) {
	if (place[row][col] != ' ') break;
    }
    --row;			// back up to last space
    if (row <0) {
	cout << "All spaces in column " << col << " are full\n";
    } else {
	cout << "placed " << player << " at " << row << "," << col << '\n';
	place[row][col] = player;
    }
    return row;
}

/*Main function*/
int
main()
{
    start_game();
}