5
\$\begingroup\$

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 <fstream>

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<n; i++) {
    bool A = false;
    bool B = false;
    bool AB = false;
        // load data
        for(short c=0; c<5; c++) {
            for(short r=0; r<5; r++) {
                source >> 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
                                B = true;
                        }
                    } else
                        streak = 1;
                }
            }
        } else
            AB = true;

        //check diagonal winner---------------------------------
        if(AB == false) {
            short streak2;
            short streak3;
            short streak4;
            for(short j=0; j<4; j++) {
                streak = 1;
                streak2 = 1;
                streak3 = 1;
                streak4 = 1;
                for(short i=0; i<5-j; i++) {
                    if(board[i+j][i] == board[i+j+1][i+1]) {
                        streak++;
                        if(streak == 3)
                            if(board[i+j][i] == 'A')
                                A = true;
                            else
                                B = true;
                    } else
                        streak = 1;



                    if(board[i-1][i+j] == board[i][i+1+j]) {
                        streak2++;
                        if(streak2 == 3)
                            if(board[i-1][i+j] == 'A')
                                A = true;
                            else
                                B = true;
                    } else
                        streak2 = 1;



                    if(board[5-i-j][i-1] == board[5-i-1-j][i]) {
                        streak3++;
                        if(streak3 == 3)
                            if(board[5-i-j][i-1] == 'A')
                                A = true;
                            else
                                B = true;
                        } else
                            streak3 = 1;



                    if(board[5-i][i+j] == board[5-1-i][i+1+j]) {
                        streak4++;
                        if(streak4 == 3)
                            if(board[5-i][i+j] == 'A')
                                A = true;
                            else
                                B = true;
                    } else
                        streak4 = 1;
                }

                if(A == true && B == true) {
                    AB = true;

                }
            }
        }
        //---------------------------------------------

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

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

    }

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

    return 0;
}

Is it correct algorithm in my code?

\$\endgroup\$
  • \$\begingroup\$ Your code works. Is that correct? \$\endgroup\$ – Nic Hartley Mar 21 '16 at 21:12
  • \$\begingroup\$ It works, but I'm not sure fully (exceptions?) \$\endgroup\$ – Piotr K. Mar 21 '16 at 21:15
  • \$\begingroup\$ That's fine. If your code works as far as you know, but you'd like help finding edge cases/bugs and fixing them, that's on-topic – as long as the code itself is functional. \$\endgroup\$ – Nic Hartley Mar 21 '16 at 21:17
6
\$\begingroup\$

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<5; c++) {
            for(short r=0; r<5; r++) {
                source >> 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 can validate the error cases in the documentation.

What I would have liked to see.

class Board
{
      // STUFF;
      friend std::istream& operator>>(std::istream& stream, Board& data);
};
int main()
{
    std::ifstream  input("source.in");

    int gameCount;
    input >> gameCount;

    Board  game;
    for(;gameCount > 0 && input >> game;--gameCount)
    {
        std::cout << game.findWinners();
    }
}
\$\endgroup\$
  • \$\begingroup\$ > Don't return 0 from main. Thats something that I believed until I start programming on PS4. PS4 ABI convention rise a signal (that trigger a crash dump) if you exit from a non void function without return something. :( \$\endgroup\$ – ilmale Mar 23 '16 at 17:16
  • 1
    \$\begingroup\$ @ilmale: That is true for all functions apart from main(). main is special and the compiler will automatically plant a return 0 at the end of the function if you don't specifically do it. BUT only for main. In all other functions this is undefined behavior. So you PS4 ABI is correct and standard compliant. \$\endgroup\$ – Martin York Mar 23 '16 at 22:01

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.