C++ Connect-four game with little AI (Version two)

Question

Here is the improved code

#include <iostream>
#include <sstream>
#include <string>
#include <random>
#include <vector>
#include <iomanip>
enum class ending
{
 win ,
 lose ,
 tie
};
enum class States
{
 Human ,
 robot ,
 blank
};
enum class command
{
 deletE ,
 move
};
class BOARD {
public:
 std::vector<States> Cells;
 BOARD(States type , int x , int y);
 bool wincon(const std::vector<States>& rboard, int coordx, int coordy);
 
};
//functions
bool legal(const std::vector<States>& Cells, int choice);
void printer(int x, int y, std::vector<States>& Cells, char skin = 'K');
void instructions();
void newpage();
ending mainloop(int realx, int realy , char skin, std::vector<States>& Cells, BOARD board);
void moving(int choice, std::vector<States>& Cells, int cols, command decide , States player , int limit);
int Ai(std::vector<States>& Cells, int coordx, int coordy , BOARD board );
int main()
{
 srand(static_cast<unsigned int>(time(0)));
 //elements
 int realx, realy;
 char skin;
 //asking player for input and intro
 std::cout << "\t\t 4-CONNNECT \n\t________________________\n";
 std::cout << "enter x cordinate please:";
 std::cin >> realx;
 std::cout << "enter y cordinate please:";
 std::cin >> realy;
 BOARD board(States::Human, realx, realy);
 printer(realx, realy, board.Cells);
 instructions();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 std::cin.get();
 newpage();
 //get player skin (letter) and pass it into game loop
 std::cout << "enter ur skin please:";
 std::cin >> skin;
 newpage();
 //test
 mainloop(realx, realy, skin, board.Cells, board);
}
//classes function defintion
BOARD::BOARD(States type , int x , int y)
{
 for (int i = 0; i < x; i++)
 {
 for (int j = 0; j < y; j++)
 {
 Cells.push_back(States::blank);
 }
 }
}
//function defintion
void printer(int x, int y, std::vector<States>& Cells , char skin )
{
 int boardnumber = 0;
 for (int i = 0; i < x; i++)
 {
 for (int j = 0; j < y; j++)
 {
 switch (Cells[boardnumber])
 {
 case States::Human:
 std::cout << skin;
 std::cout << std::setw(2);
 break;
 case States::robot:
 std::cout << "B";
 std::cout << std::setw(2);
 break;
 case States::blank:
 std::cout << boardnumber+1;
 if (boardnumber+1 < 10) {
 std::cout << std::setw(2);
 }
 break;
 default:
 break;
 }
 std::cout << "|";
 boardnumber++;
 }
 std::cout << "\n";
 }
}
void instructions()
{
 std::cout << "\ninstructions\n_________________\n";
 std::cout << "\n";
 std::cout << "just search google for instructions for this game :)\n";
}
void newpage()
{
 for (int i = 0; i < 50; i++)
 {
 std::cout << "\n";
 }
}
ending mainloop(int realx, int realy , char skin, std::vector<States>& Cells, BOARD board)
{
 //elements
 int lobbyposition;
 int blank = 1;
 int limit = realx * realy;
 int userchoice , aichoice;
 //first or last
 std::cout << "start first(yes = 0 or no = 1):";
 std::cin >> lobbyposition;
 newpage();
 
 //loob
 while (blank != limit) {
 if (lobbyposition % 2 == 0) {
 printer(realx, realy, Cells, skin);
 std::cout << "enter the position:";
 std::cin >> userchoice;
 moving(userchoice , Cells, realy, command::move, States::Human , limit);
 newpage();
 printer(realx, realy, Cells, skin);
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 std::cin.get();
 newpage();
 blank++;
 lobbyposition++;
 if (board.wincon(Cells, realx, realy))
 return ending::win;
 }
 else if (lobbyposition % 2 != 0) {
 aichoice = Ai(Cells, realx, realy, board );
 printer(realx, realy, Cells, skin);
 std::cout << "\nmy result is :" << aichoice;
 newpage();
 printer(realx, realy, Cells, skin);
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 std::cin.get();
 newpage();
 blank++;
 lobbyposition++;
 if (board.wincon(Cells, realx, realy))
 return ending::lose;
 }
 }
 
}
bool BOARD::wincon(const std::vector<States>& rboard, int coordx, int coordy)
{
 bool check = false;
 int equ1, equ2 , counter;
 counter = 0;
 States startvalue = rboard[counter];
 for (int x = 0; x < coordx; x++)
 {
 for (int y = 0; y < coordy; y++)
 {
 startvalue = rboard[counter];
 int possiblex[4][2] = { {0 , 1} , {1 , 0} , {1 , -1} , {1 , 1} };
 if (startvalue != States::blank) {
 //checkloop
 for (int j = 0; j < 4; j++)
 {
 check = true;
 int i = 0;
 for (int b = 1; b < 4; ++b) {
 equ1 = (x + (b * possiblex[j][i]));
 equ2 = (y + (b * possiblex[j][i + 1]));
 if (equ1 < 0 || equ1 == coordx) {
 check = false;
 break;
 }
 else if (equ2 < 0 || equ2 == coordy) {
 check = false;
 break;
 }
 else
 {
 if (rboard[equ2 + equ1 * coordy] != startvalue) {
 check = false;
 }
 }
 }
 if (check == true) {
 return check;
 }
 }
 
 }
 counter++;
 }
 }
 return check;
}
bool legal(const std::vector<States>& Cells, int choice)
{
 int counter = 1;
 for (States loop : Cells){
 if (counter == choice)
 {
 if (loop != States::blank) {
 return false;
 }
 else {
 return true;
 }
 }
 counter++;
 }
 return false;
}
void moving(int choice, std::vector<States>& Cells, int cols, command decide, States player , int limit)
{
 int position = choice+cols;
 while (legal(Cells, position) == true)
 {
 choice += cols;
 position += cols;
 }
 choice -= 1;
 if (decide == command::move) {
 Cells[choice] = player;
 }
 else if ( decide == command::deletE)
{
 if (legal(Cells, (choice+1)) != true){
 Cells[choice] = States::blank;
 }
 else {
 Cells[choice+cols] = States::blank;
 }
 }
}
int Ai(std::vector<States>& Cells,int coordx, int coordy , BOARD board )
{
 //checkwin and stop win
 int limit = coordx * coordy;
 int counter = 1;
 while (counter < limit) {
 if (legal(Cells, counter)) {
 //check win
 moving(counter, Cells, coordy, command::move, States::robot, limit);
 if (board.wincon(Cells, coordx, coordy) == true) {
 return counter;
 }
 moving(counter, Cells, coordy, command::deletE, States::robot, limit);
 }
 counter++;
 }
 counter = 1;
 while (counter < limit) {
 if (legal(Cells, counter)) {
 //check enemy
 moving(counter, Cells, coordy, command::move, States::Human, limit);
 if (board.wincon(Cells, coordx, coordy) == true) {
 moving(counter, Cells, coordy, command::deletE, States::Human, limit);
 moving(counter, Cells, coordy, command::move, States::robot, limit);
 return counter;
 }
 moving(counter, Cells, coordy, command::deletE, States::Human, limit);
 }
 counter++;
 }
 //random number generatror (sry was lazy to updata to c++ 11, its too complicated for my brain)
 for (int i = 0; i < 1000; i++)
 {
 counter = (rand() % limit)+1;
 if (legal(Cells , counter)){
 moving(counter, Cells, coordy, command::move, States::robot , limit);
 return counter + 1;
 }
 }
}

That's an improved version from my first 4-connect post. I added classes and enums and no multidimentional vectors I think.

I also made the code look better and removed childish names.

• JDługosz: My first impression is that many of the things you list under "functions" ought to be members. Don't pass in a vector of states... make it a member function of the BOARD class.

Answer 1

It's good to see the improvements you've made to your code! There's still more that can be done better though, some of which I'll list below.

Use a consistent code style

The style of your code has lots of inconsistencies. Some of them are related to the use of spaces (especially around commas) and empty lines. A code formatting tool, like Artistic Style or ClangFormat, can fix these issues in an automated way for you. Or perhaps even your editor or IDE has a function to format the code for you. Which style you use is not as important as being consistent.

Even more important is the choice of names for types, variables and functions. Try to be consistent with the way you capitalize them. I recommend you use the following rules of thumb:

• Types (structs, classes and enums) are in PascalCase: every individual word in the name of the type starts with a capital. So:
  • ending -> Ending
  • command -> Command
  • BOARD -> Board
• Functions and variables in either snake_case or camelCase. So if you go for snake_case, then:
  • wincon() -> win_con()
  • mainloop() -> main_loop()
  • coordx -> coord_x
  • and so on.
• Enum values are in all UPPER_CASE, so:
  • win -> WIN
  • Human -> HUMAN
  • and so on.

Naming things

Some names for functions and variables could be better. Make a distinction between x and y coordinates, and between width and height. So the constructor of BOARD for example should use width and height as the parameters, and then it can use x and y as the loop indices, like so:

Board::Board(int width, int height)
{
 for (int y = 0; y < height; ++y)
 {
 for (int x = 0; x < width; ++x)
 {
 Cells.push_back(States::BLANK);
 }
 }
}

In printer(), there is a variable boardnumber. However, there's only one board in the game, so this name is a bit confusing. Instead it's just the index into Cells. In that case, I would name it cell_index, or perhaps just index since it's a rather small function and it should be rather clear.

You are also being inconsistent with naming. In some other functions you used variables named counter and position for the index into Cells.

Inside main() and mainloop(), you use realx and realy for what probably should be named width and height.

Some other rules of thumb:

• Use singular for most things, except for collections of things. So:
  • States -> State
• Prefer verbs for functions, nouns for variables. For functions returning a yes/no answer, prefix it with is_. So:
  • legal() -> is_legal()
  • printer() -> print()
  • instructions() - show_instructions()
  • and so on.

Avoid redundant function arguments

Some functions take arguments that are either never used (like type in BOARD::BOARD()), or are redundant. For example, mainloop() takes both a reference to Cells, and has a paramter board. But the board already has the vector of cells as a member variable. In this case, remove the reference to the cells, and just pass the board as a reference:

Ending main_loop(int width, int height, char skin, Board &board)
{
 ...
 printer(width, height, board.Cells, skin);
 ...

Even better if all functions take references to a Board instead of a reference to a std::vector<States>. But it can be improved a lot more:

Make BOARD a better class

At the moment, your class BOARD doesn't do much; it just holds the cells, and has only one function that checks for a winning condition. However, this class doesn't know its own width and height, it doesn't know how to print itself, it can't tell if a given move is valid, and so on. Some of your regular functions should be made member functions of this class. That way, you don't have to pass the width, height, and a reference to a BOARD or to a std::vector<States> to them. It might look like this:

class Board {
public:
 const int width;
 const int height;
 std::vector<State> cells(width * height, State::BLANK);
 Board(int width, int height);
 void print() const;
 void make_move(State player, int column);
 bool is_legal_move(int column) const;
 bool is_winning_condition() const;
};

And then the implementation of the functions can look like this:

Board::Board(int width, int height): width(width), height(height) {}
void Board::print() const {
 int index = 0;
 for (int y = 0; y < height; ++y) {
 for (int x = 0; x < width; ++x) {
 switch (cells[index]) {
 ...
 }
 std::cout << '|';
 index++:
 }
 std::cout << '\n';
 }
}
bool Board::is_legal(int column) const {
 return cells[column] == State::blank;
}
...

Don't just make every function a member function of class Board though. Functions that have nothing to do with the Board should of course not be in it, but also functions like Ai() might be a bad candidate; while they do interact with the board, an AI is not part of the board, it's just one of the players. So for functions like that, just pass a reference like before.

Use C++11's random number generators

I mentioned this in the previous review, but I'll repeat this again (for the benefit of other readers): the srand() and rand() functions come from C, and are of low quality. Consider using the C++11 random number functionality. See this StackOverflow post for how to use it.

It's really not that hard to use. First, create the following variables:

std::random_device rdev;
std::mt19937 rng(rdev());

These basically replace the call to srand() at the top of main(). Then to use it to generate a random numbers between 1 and limit + 1 for example, write:

std::uniform_int_distribution distrib(1, limit + 1);
for (int i = 0; i < 1000; ++i) {
 counter = distrib();
 ...
}

Here the variable distrib basically is a replacement for rand() that is specialized to provide integers perfectly distributed between the two limits you give it, and by "calling" it you will get one of those integers out.

Overload operator<< to print the board

The C++ way to print things is to write std::cout << something. It would be nice if you could just write:

Board board(7, 6);
std::cout << board;

This is actually not that hard to do in C++, see for example this tutorial. Instead of having a function printer() or member function print(), you have to create a friend operator<<() function for your class, which looks like this:

class Board {
 ...
 friend std::ostream& operator<<(std::ostream& out, const Board& board);
};
std::ostream& operator<<(std::ostream &out, const Board& board) {
 int index = 0;
 for (int y = 0; y < height; ++y) {
 for (int x = 0; x < width; ++x) {
 switch (cells[index]) {
 ...
 }
 out << '|'; // We just had to replace std::cout with out here!
 index++:
 }
 out << '\n';
 }
 return out; // Don't forget to return out
}

• Ahmed Elsayed: sorry for the late reply, was busy in school.
• Ahmed Elsayed: I will try to correct myself this information

Answer 2

Your mainloop and Ai functions take a BOARD object by value, which will make a copy. You can pass those as const BOARD &board to avoid this. And since the board object already has the Cells data, you shouldn't need to pass that in as a separate parameter.

From your comment near the end, you're already aware you should use the random number facilities provided by the language. You're including the <random> header, and you should make use of it. It won't make much difference for a simple game like this, but take the opportunity to learn it now so you'll know it later when it is important.

The coordinate naming seems to be backwards, as you use the y value to index horizontally and the x to index vertically. This makes it a bit harder for someone else to understand, because usually X is horizontal and Y is vertical.

The BOARD constructor is doing a lot of unnecessary work, and it doesn't use the type parameter at all. You can simplify it to

BOARD::BOARD(int x, int y): Cells(x * y, States::blank)
{
}

The x and y values should be stored in BOARD, because they need to be known to properly access Cells.

printer can be a member of BOARD, as can several of your other functions (anything you pass Cells to would be a good candidate). Your setw calls seem to be in the wrong place; they should come before the value whose width you want to set. The boardnumber+1 < 10 comparison can be simplifed to boardnumber < 9, although if you move the setws you'll wnat to compare boardnumber < 10. Since you don't use the i or j values, you can get rid of them. Use a range-based for (for (auto c: Cells)), increment a counter to track the column and output a newline every x columns.

Rather than a loop to output newlines one at a time, newpage can make use of a static char array (manually filled with newlines) or a temporary string (std::cout << std::string(50, '\n');). Consider changing that 50 to a named constant.

mainloop doesn't return a value if the board fills up (blank == limit), which is undefined behavior. You should return something to indicate that the game was a draw (a tie). The else if condition is unnecessary, because it will always be true (because if lobbyposition % 2 == 0 is false, lobbyposition % 2 != 0 will be true).

In wincon, declare variables as you need them, not a the function start. possiblex can be static const int to avoid constructing the array for every position. You also seem to be checking the wrong places (your array has down, right, up and left, down and left). You should be using { {0, 1}, {0, -1}, {1, 0}, {-1, 0} }. The bounds checking looks for underflow but not going off the right side or bottom of the grid. You need to check for it with equ1 >= coordx and equ2 >= coordy in the appropriate place. You could add a break; if the rboard check sets check to false.

The legal function could use a better name. It does a lot of work just to check one value. Use the indexing to look up the value in Cells. To make that easy, add an indexing function to return the value at a particular cell, and use it here and in wincon.

Including some comments to explain what some of these functions do would help with the readability. What is moving supposed to be doing? It looks like it searches down the chose column to find where the marker ends up, then sets it. Changing legal to take both an x and y value, and using the indexing function can simplify this and avoid the need for adding or subtracting 1 from choice.

What is the loop at the end of Ai supposed to do? There are only a limited number of moves that can be made. If it gets that far, and that loop runs to completion, you don't return anything from Ai which can result in a string value in aichoice back at the callsite. Since you don't do anything with that value other than print it out, there is no real harm but the potential is there for Something Bad to happen if that usage changes. (This could happen if, for example, you separate out the Ai logic to one function to decide the move, and another one shared between the Ai and human player that makes the move.)