I finally made it! What do you think?

Minimacfox (71)
Finally created my Connect 4 game! What do you guys think? Do I use too many if statements in my winCondition?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
 
#include <iostream>
#include <cstdlib>
#include <ctime>
using namespace std;

void timer(unsigned short i){
clock_t delay = i*CLOCKS_PER_SEC;
clock_t start = clock();
    while(clock() - start < delay);
}

class ConnectFour{
public:
    ConnectFour(){markerX = 'X', markerY = 'Y';}
    void assignBoard(char multiArray[][7]){
        for(int r=0; r<6; r++){
            for(int c=0; c<7; c++){
                multiArray[r][c] = 'O';
            }
        }
    }
    void displayBoard(char multiArray[][7]){
        for(int r=0; r<6; r++){
            cout << "           ";
            for(int c=0; c<7; c++){
                cout << multiArray[r][c] << "|";
            }
            cout << endl;
        }
        cout << "           " << "1 2 3 4 5 6 7" << endl;
    }
    void turn(unsigned short& loop){
        cout << endl;
            if(loop%2 == 0)
                cout << "            " << "Player X: ";
            else
                cout << "            " << "Player Y: ";
    }
    bool errHandle(int& value){
        if(!cin){
            cout.put('\a');
            system("cls");
            system("color 1A");
            cout << "ERROR: Non-integer value has been registered!\n\nTERMINATING\n";
            exit(EXIT_FAILURE);
        }
        else if(value <= 0 || value > 7){
            system("cls");
            cout << "Please only enter values between 1 and 7\n";
            timer(2);
            system("cls");
            return true;
        }

    }
    void pos(char multiArray[][7], int& value, unsigned short& loop){
        char currentPlayerMarker;
            if(loop%2 == 0)
                currentPlayerMarker = markerX;
            else
                currentPlayerMarker = markerY;
            for(int row = 5; row >=0; row--){
                if(multiArray[row][value-1] == 'O'){
                    multiArray[row][value-1] = currentPlayerMarker;
                break;
            }
        }
    }
    bool boardLimit(char multiArray[][7], int& value){
        short counter = 1;
            for(int c=0; c<6; c++){
                if(markerX == multiArray[0][c] && value == counter || markerY == multiArray[0][c] && value == counter){
                    system("cls");
                    cout << "Row " << value << " is filled, pick another.\n";
                    timer(3);
                    system("cls");
                    return true;
            }
            counter++;
        }
    }
    void winCondition(char multiArray[][7]){
    bool checkX = false, checkY = false;
        /* Checking condition vertically */
        for(int row=5; row>=0; row--){
            for(int column=0; column<4; column++){
                if(multiArray[row][column] == markerX && multiArray[row][column+1] == markerX && multiArray[row][column+2] == markerX && multiArray[row][column+3] == markerX){
                    checkX = true;
                    break;
                }
                else if(multiArray[row][column] == markerY && multiArray[row][column+1] == markerY && multiArray[row][column+2] == markerY && multiArray[row][column+3] == markerY){
                    checkY = true;
                    break;
                }

            }
            if(checkX == true || checkY == true)
                break;
        }
        /* Checking condition horizontally */
        for(int column=0; column<6; column++){
            for(int row=5; row>2; row--){
                if(multiArray[row][column] == markerX && multiArray[row-1][column] == markerX && multiArray[row-2][column] == markerX && multiArray[row-3][column] == markerX){
                    checkX = true;
                    break;
                }
                else if(multiArray[row][column] == markerY && multiArray[row-1][column] == markerY && multiArray[row-2][column] == markerY && multiArray[row-3][column] == markerY){
                    checkY = true;
                    break;
                }
            }
            if(checkX == true || checkY == true)
                break;
        }
        /* Checking condition obliquelly */
        for(int row=5; row>=0; row--){
            for(int column=0; column<=3; column++){
                if(multiArray[row][column] == markerX && multiArray[row-1][column+1] == markerX && multiArray[row-2][column+2] == markerX && multiArray[row-3][column+3] == markerX){
                    checkX = true;
                    break;
                }
                else if(multiArray[row][column] == markerY && multiArray[row-1][column+1] == markerY && multiArray[row-2][column+2] == markerY && multiArray[row-3][column+3] == markerY){
                    checkY = true;
                    break;
                }
            }
            if(checkX == true || checkY == true)
                break;
        }
        /* Checking condition reversed obliquelly */
        for(int row=5; row>=0; row--){
            for(int column=6; column>=3; column--){
                if(multiArray[row][column] == markerX && multiArray[row-1][column-1] == markerX && multiArray[row-2][column-2] == markerX && multiArray[row-3][column-3] == markerX){
                    checkX = true;
                    break;
                }
                else if(multiArray[row][column] == markerY && multiArray[row-1][column-1] == markerY && multiArray[row-2][column-2] == markerY && multiArray[row-3][column-3] == markerY){
                    checkY = true;
                    break;
                }
            }
            if(checkX == true || checkY == true)
                break;
        }
        if(checkX == true){
            system("cls");
            system("color 2B");
            displayBoard(multiArray);
            cout << "\n           Player X wins!\n";
            exit(EXIT_SUCCESS);
        }
        else if(checkY == true){
            system("cls");
            system("color 2B");
            displayBoard(multiArray);
            cout << "\n           Player Y wins!\n";
            exit(EXIT_SUCCESS);
        }
    }

private:
    char markerX, markerY;
};

main(){
char board[6][7];
unsigned short loop = 0;
int value;
ConnectFour game;
game.assignBoard(board);
    while(loop < 42){
        game.displayBoard(board);
        game.turn(loop);
        (cin >> value).get();
            if(game.errHandle(value) == true)
                continue;
            else if(game.boardLimit(board, value) == true)
                continue;
        game.pos(board, value, loop);
        game.winCondition(board);
        loop++;
        system("cls");
    }
system("cls");
game.displayBoard(board);
cout << "\n            IT'S A DRAW\n";
}
closed account (3qX21hU5)
Congrats!

I do have some suggestions though.

1) Move the definitions of the members functions outside of the class.

2) main() always returns int. You main function is missing a return type.

3) Take care of all the warnings from the compiler like putting parenthesis around the '&&' in the || on this
if(markerX == multiArray[0][c] && value == counter || markerY == multiArray[0][c] && value == counter) .

4) Some of your member functions might be able to reach the end of the function without returning anything. Like in bool boardLimit(char multiArray[][7], int& value). What if it reaches the end of that function? What will it return?

Them are just some things I see from the a little search. Might take a better in depth look through it later.

Minimacfox (71)
Thanks for the opinion ;)

The reason why I didn't put "int" infront of main(), is because in Code::blocks it isn't neccessary to do it.

Also what do you mean with 1)? Moving the definitions of the members functions? Like prototyping them outside of the class?
giblit (3750)
Just because something isn't "necessary" doesn't mean you shouldn't do it that's just being lazy =p
Minimacfox (71)
Programmers ARE lazy ;)
keskiverto (10399)
Also what do you mean with 1)? Moving the definitions of the members functions? Like prototyping them outside of the class?


You have:
1
2
3
4
5
6
7
 
class Foo {
public:
  void func() const
    {
      std::cout << "Hello\n"
    }
};


Separate it to:
1
2
3
4
5
6
7
8
9
10
11
 
// Definition
class Foo {
public:
  void func() const;
};

// Implementation
void Foo::func() const
{
  std::cout << "Hello\n"
}

Then you can move the implementation into a separate Foo.cpp source file and save the Definition as Foo.h header file (but add inclusion guards).

Every project that uses class Foo can then simply include the Foo.h and link in the object file created from Foo.cpp.
andywestken (4094)
Misc other comments...

I agree with all the above, including the one about int main() -- even though the GCC compiler allows you to be lazy, for backward compatibility reasons, you should get into the habit of usig the ANSI compliant forms of main: either int main() or int main(int argc, char* argv[]), with a return!

1. Long lines of code should be folded (this helps make them more readable not only in your IDE but here on cplusplus.com)

1
2
3
4
5
6
7
8
 
        /* Checking condition obliquelly */
        for(int row=5; row>=0; row--){
            for(int column=0; column<=3; column++){
                if(multiArray[row][column] == markerX && multiArray[row-1][column+1] == markerX &&
                   multiArray[row-2][column+2] == markerX && multiArray[row-3][column+3] == markerX){
                    checkX = true;
                    break;
                }


or even

1
2
3
4
5
6
7
8
9
10
 
        /* Checking condition obliquelly */
        for(int row=5; row>=0; row--){
            for(int column=0; column<=3; column++){
                if(multiArray[row][column] == markerX &&
                   multiArray[row-1][column+1] == markerX &&
                   multiArray[row-2][column+2] == markerX &&
                   multiArray[row-3][column+3] == markerX){
                    checkX = true;
                    break;
                }


rather than

1
2
3
4
5
6
7
 
        /* Checking condition obliquelly */
        for(int row=5; row>=0; row--){
            for(int column=0; column<=3; column++){
                if(multiArray[row][column] == markerX && multiArray[row-1][column+1] == markerX && multiArray[row-2][column+2] == markerX && multiArray[row-3][column+3] == markerX){
                    checkX = true;
                    break;
                }


(A limit of 80 characters per line was common in the past, when screens were smaller and people mostly used console-based text editors. In the days of IDEs and large screens, you can use slightly longer rows, but still want to be able to see your code, the debugger output, and you app's window all at the same time -- and be able to see the whole line of code without scrolling.)

2. C++ coders usually use

if(checkX){

rather than

if(checkX == true){

as checkX is already a boolean, you don't need to compare it against true. The result of operator== is a bool value. e.g. value = (checkZ == true) --> true or false

also, it save a bit of typing (you did say you're lazy!)

3. Some of the variable and function names could be improved.

All variable names should say what they are, function names what they do.

e.g. checkX -> check what?

hasLineX might be better?

And "displayBoard" is a good function name, but "turn" is not. Turn what?

4. You should use pre-increment rather than post-increment

There are cases where the post-increment for performs worse, so it's a good habit to use ++i rather than i++

Same deal for decrement.

5. You could replace literals with meaningful contants

const int numRows = 6;
const int numCols = 7;

char board[numRows][numCols];

rather than

char board[6][7];

and then use them in you loops.

6. From a object oriented point of view, rather than a C++ syntax point of view... If you plan on a version 2 of you game, you should consider factoring out the board specific code into its own (Board) class, rather than using the char array directly (ie encapsulate the board).

Andy
Last edited on
giblit (3750)
I thought you use int main( int argc , char **argv ) instead of *argv[] if you don't need access to items in argv think I read that somewhere on the internet could be wrong though was a while back
Last edited on
Topic archived. No new replies allowed.