HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

TicTacToe 5x5 win or draw algorithm

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
win5x5tictactoedrawalgorithm

Problem

I have file which contains a lot of boards from TicTacToe 5x5.

First line says how many boards this file contains (n*5x5).

We check the winner in Horizontals, Verticals and Diagonals lines.

Player A or B win, when has streak of minimum 3 letters (can be 4, 5) on the board. for example:


ABABA

AABAA

AABAA
ABABA
AABAA

Draw is when Player A and B has streak of minimum 3 letters (can be 4, 5). for example:


AAAAA

AAAAA

BAAAA

ABAAA

AABAA

Draw is also when nobody has streak minimum of 3.

source.in - example source file

2
AABBA
BAAAB
AAABA
ABAAB
BAAAB
AAAAA
AAAAA
BAAAA
ABAAA
AABAA


output.out - example correct output

A win
draw


And my code.

```
#include

using namespace std;

int main() {

int n;
char board[5][6];

ifstream source;
source.open("source.in");

ofstream output;
output.open("output.out");

source >> n;

for(int i=0; i> board[c][r];
}
}

// check Winner
//
// check Horizontal Winner
short streak = 1;
for(short y=0; y<5; y++) {
streak = 1;
for(short x=0; x<4; x++) {
if(board[y][x] == board[y][x+1]) {
streak++;

if(streak == 3) {
if(board[y][x] == 'A')
A = true;
else
B = true;
}
} else
streak = 1;
}
}

// check Vertical Winner
if(!((A == true && B == true) || (A == false && B == false))) {
for(short x=0; x<5; x++) {
streak = 1;
for(short y=0; y<4; y++) {
if(board[y][x] == board[y+1][x]) {
streak++;

if(streak == 3) {
if(board[y][x] == 'A')
A = true;
else

Solution

Not C++ like

You have basically build a C application and compiled it with a C++ compiler. The few C++ features you have used could just as easily have been C and it would have worked the same.

What I would have expected to see is at least a class that represents a board. The board would know how to read itself from a stream and then analyze itself with a decent interface from the program.
Self Documenting code

This is a desgin principle. By naming your functions and variables appropriately you can make your code self documenting and thus easy to read/understand/maintain.

But you have thrown all your code into a single function making it very hard to read. I mean rather than putting this directly into the code:

// load data
        for(short c=0; c> board[c][r];
            }
        }


You could have written:

loadBoardFromStream(source);


That is much easier to read than a doubly nested loop. I don't actually have to go to that piece of code and read it to understand what it does (because the function name tells me what it does).
C++ code review
Using namespace

Don't do this.

using namespace std;


See Why is “using namespace std” in C++ considered bad practice?.

Basically anything beyond a ten line program it will start giving you problems (when you write C++ and not C). Consider it a bad habit. If you continue to use it that habit will become enforced and you will accidently use it when you don't mean to and it will cause a problem.

Also the standard namespace is only three characters long for a reason. It so that prefix things from this namespace is not a huge burdon.

std::cout << "Testing\n";

 std::ifstream file("Plop.txt");

 // See not that much hassle to use `std::` as a prefix.


Don't Declare variablesearly

int n;
    char board[5][6];


Declare variables just before they are first used. This helps in code readability. Because the type of the object (very important in C++) is where the object is being used and thus you can verify it.

Also constructors are called at the point where they are declared. No point in paying for the cost of constructing an object when you never use it. So declare it close to the point where you first use it.
Use constructors

ifstream source;
    source.open("source.in");


The std::ifstream constructor has a version that takes the name of the file.

std::ifstream source("source.in");


Use it rather than taking two steps to open the file.
Prefer prefix increment

for(int i=0; i<n; i++) {


For integers it makes no difference the prefix or postfix increment. But you can not gurantee that some type will stay a particular type for the whole history of its lifespan. Code is maintained updated patched and fixed.

For User defined types the default implementation of prefix and postfix increment the postfix increment is more expensive. So prefer to use the prefix version then you will always be using the correct version even if the type changes underneath your code.

for(int i=0; i<n; ++i) {


Do you really need this variable?

bool AB = false;


Seems to me that you can accidentally get into a mixed state if there is a bug in your code.

This test here:

if(AB == true){
            output << "draw";
        }
        else if (A == true) {
            output << "A win";
        } else if(B == true) {
            output << "B win";
        }


I would have written it as:

if(A == B) {
            output << "draw";
        }
        else if (A == true) {
            output << "A win";
        } else
            output << "B win";
        }


Always use {} around sub-blocks.

if(i<n-1)
            output << endl;


Yep it is allowed. But there are certain conditions that you as a novice may not spot that make a single statement actually become two statements. As a result one will be part of the if sub statement while the other becomes the next statement to execute. So always use {} to make things explicit.

if(i<n-1) {
            output << endl;
        }


Prefer '\n' to std::endl

The difference between the two is that std::endl also flushes the stream buffer. This sounds good; but the stream buffer will automatically flush when it needs to. Therefore forcing the flush just makes the code more inefficient.
Don't manually close a file

source.close();
    output.close();


The destructor of the file stream object will automatically close the stream for you. It will also catch and drop any errors (this includes exceptions). You should only close the stream manually if you want to detect a close() failure and take action based on that failure.
Don't return 0 from main.

return 0;


If the only thing main returns is 0 then don't return anything. This documents that the application has no failure cases. If you return 0 at the end I start looking through your code to find the situation were it returns something else to indicate an error so that I

Code Snippets

// load data
        for(short c=0; c<5; c++) {
            for(short r=0; r<5; r++) {
                source >> board[c][r];
            }
        }
loadBoardFromStream(source);
using namespace std;
std::cout << "Testing\n";

 std::ifstream file("Plop.txt");

 // See not that much hassle to use `std::` as a prefix.
int n;
    char board[5][6];

Context

StackExchange Code Review Q#123483, answer score: 6

Revisions (0)

No revisions yet.