Find total of non-attacked positions in a Chess board - UVA 10284

Question

The input is until EOF, give in the FEN notation. The output should be the count of non-attacked positions. My code passes in all test cases in uDebug, but it doesn't pass on the hidden cases.

All the problem description can be read here.

#include <bits/stdc++.h>

using namespace std;

int main()
{
    string read;

char board[8][8], board_aux[8][8];

while (cin >> read)
{
    int i = 0, j = 0;

    memset(board, '0', sizeof(board[0][0]) * 8 * 8);
    memset(board_aux, '0', sizeof(board_aux[0][0]) * 8 * 8);

    for (int k = 0; k < (int) read.size(); k++)
    {
        char c = read[k];
        if (c == '/')
        {
            i++;
            j = 0;
        }
        else if (c >= '0' and c <= '9')
            j += c - '0';
        else
        {
            board[i][j] = c;
            j++;
        }
    }

    for (i = 0; i < 8; i++)
    {
        for (j = 0; j < 8; j++)
        {
            char c = board[i][j];

            if (c == '0') continue;

            board_aux[i][j] = '1';

            if (c == 'p')
            {
                if (i + 1 < 8 and j + 1 < 8)
                    board_aux[i + 1][j + 1] = '1';
                if (i + 1 < 8 and j - 1 > -1)
                    board_aux[i + 1][j - 1] = '1';
            }
            else if (c == 'P')
            {
                if (i - 1 > -1 and j + 1 < 8)
                    board_aux[i - 1][j + 1] = '1';
                if (i - 1 > -1 and j - 1 > -1)
                    board_aux[i - 1][j - 1] = '1';
            }
            else if (c == 'K' or c == 'k')
            {
                if (i + 1 < 8)
                    board_aux[i + 1][j] = '1';
                if (i - 1 > -1)
                    board_aux[i - 1][j] = '1';
                if (j + 1 < 8)
                    board_aux[i][j + 1] = '1';
                if (j - 1 > -1)
                    board_aux[i][j - 1] = '1';
                if (i + 1 < 8 and j + 1 < 8)
                    board_aux[i + 1][j + 1] = '1';
                if (i + 1 < 8 and j - 1 > -1)
                    board_aux[i + 1][j - 1] = '1';
                if (i - 1 > -1 and j + 1 < 8)
                    board_aux[i - 1][j + 1] = '1';
                if (i - 1 > -1 and j - 1 > -1)
                    board_aux[i - 1][j - 1] = '1';
            }
            else if (c == 'N' or c == 'n')
            {
                if (i - 2 > -1 and j + 1 < 8)
                    board_aux[i - 2][j + 1] = '1';
                if (i - 2 > -1 and j - 1 > -1)
                    board_aux[i - 2][j - 1] = '1';
                if (i + 2 < 8 and j + 1 < 8)
                    board_aux[i + 2][j + 1] = '1';
                if (i + 2 < 8 and j - 1 > -1)
                    board_aux[i + 2][j - 1] = '1';
                if (i + 2 < 8 and j + 2 < 8)
                    board_aux[i + 1][j + 2] = '1';
                if (i + 2 < 8 and j - 2 > -1)
                    board_aux[i + 1][j - 2] = '1';
                if (i - 2 > -1 and j + 2 < 8)
                    board_aux[i - 1][j + 2] = '1';
                if (i - 2 > -1 and j - 2 > -1)
                    board_aux[i - 1][j - 2] = '1';
            }
            else if (c == 'R' or c == 'r' or c == 'Q' or c == 'q')
            {       
                for (int k = j + 1; k < 8; k++)
                {
                    if (board[i][k] != '0') break;

                    board_aux[i][k] = '1';
                }

                for (int k = j - 1; k > -1; k--)
                {
                    if (board[i][k] != '0') break;

                    board_aux[i][k] = '1';
                }

                for (int k = i + 1; k < 8; k++)
                {
                    if (board[k][j] != '0') break;

                    board_aux[k][j] = '1';
                }

                for (int k = i - 1; k > -1; k--)
                {
                    if (board[k][j] != '0') break;

                    board_aux[k][j] = '1';
                }
            }

            if (c == 'Q' or c == 'q' or c == 'B' or c == 'b')
            {
                int z = j + 1;
                for (int k = i + 1; k < 8; k++, z++)
                {
                    if (z == 8 or board[k][z] != '0') break;

                    board_aux[k][z] = '1';
                }

                z = j + 1;
                for (int k = i - 1; k > -1; k--, z++)
                {
                    if (z == 8 or board[k][z] != '0') break;

                    board_aux[k][z] = '1';
                }

                z = j - 1;
                for (int k = i + 1; k < 8; k++, z--)
                {
                    if (z < 0 or board[k][z] != '0') break;

                    board_aux[k][z] = '1';
                }

                z = j - 1;
                for (int k = i - 1; k > -1; k--, z--)
                {
                    if (z < 0 or board[k][z] != '0') break;

                    board_aux[k][z] = '1';
                }
            }
        }
    }

    int aux = 0;

    for (int i = 0; i < 8; i++)
        for (int j = 0; j < 8; j++)
            if (board_aux[i][j] == '0')
                aux++;

    cout << aux << endl;
}

return 0;
}

Input example:

PPPPPPP/8/8/8/8/8/8/ppppppp
1N1N1N1N/N1N1N1N1/1N1N1N1N/N1N1N1N1/1N1N1N1N/N1N1N1N1/1N1N1N1N/N1N1N1N1
7Q/8/8/8/8/8/8/7Q
q7/2K5/8/8/3BQ3/8/8/8

Output:

49
0
30
15

Answer 1

This is not C++

Calling this C++ is disingenuous (claiming C++11 nope). This is a C program that just happens to use a few C++ objects (std::cout and std::cin) and the type std::string. This style is commonly derisively referred to as C with Classes.

My problem here is that C++ is a style and the way you use the language is what differentiates it from C.

Code review

Non Standard

This is not a standard header.

#include <bits/stdc++.h>

You can not use this, it is not standard and you are not guaranteed that it will be available anywhere. It just happens to work for your compiler implementation.

Classic Anti Pattern

Don't use this:

using namespace std;

Books use to conserve horizontal space. But in real life you should never use this as it leads to hard to spot bugs. Always use the prefix std:: in front of standard types.

You can read all about the problems here.

Basically this is a bad habit you don't want to get started with so stop now before you use it accidentally.

One Variable Per line

The point of using a high level language is to make reading easy. The standard conventions are to declare one variable per line to increase readability.

char board[8][8], board_aux[8][8];

Memset!

OK this works. But it is only even necessary because you used old school C structures instead of C++ ones (that would have initialized the structures correctly for you). Getting the types to correctly initialize themselves is good practice as it removes accidental mistakes.

    memset(board, '0', sizeof(board[0][0]) * 8 * 8);
    memset(board_aux, '0', sizeof(board_aux[0][0]) * 8 * 8);

Don't use Cast.

    for (int k = 0; k < (int) read.size(); k++)

The C cast (int) is basically you telling the compiler to shut up about a warning and hiding it because you know better than the compiler. This is always a bad idea, the compiler is much better than you, listen to it.

If you must use a cast then use a C++ cast static_cast<int>() this is much easier to see and thus have other programmers verify.

But what you should really do is fix the code so the compiler is not generating a warning. Note: A compiler warning is logical error in your code (not necessarily against the rules of the language but an error in your logic). So fix it.

Prefer prefix increment

    for (int k = 0; k < (int) read.size(); k++)

It more usual in C++ to use prefix increment rather than suffix increment. For integers it makes no difference but for other types it can. If you use prefix increment consistently you will always have the most efficient version no matter what the type. In C++ types are really important so going in and changing the type (without changing the rest of the code) is common in maintenance.

Always use braces {}

        else if (c >= '0' and c <= '9')
            j += c - '0';
        else

It does not look like a big thing. But it can get you into trouble when you use macros. It also makes the code look more consistent.

Self documenting code

You now have a huge section of code that does different things depending on type. This would be much easier to read if you had split this up into a function for each type of piece and called the function.

            if (c == 'p') {
                checkPawn(i, j, Black);
            }
            else if (c == 'P') {
                checkPawn(i, j, White);
            }
            ... etc.

The other thing this would have done is made your code more modular and removed duplicated code (like the black and white pawn). Having repeated code means that if you find a bug in the code this means you need to fix the bug in multiple places. If your code is well partitioned into functions then a bug only needs to be fixed once.

This big if statement could be better could be better handled by a switch rather than an if.

    switch(board[i][j]) {
        case 'p':
        case 'P': checkPawn(i, j, isUpper(board[i][j]) ? White: Black); break;
        case 'r':
        case 'R': checkRook(i, j, isUpper(board[i][j]) ? White: Black); break;
        ... etc.

Simplify your code

A lot of the code for the pieces could be simplified by using loops. Or using the same loop for all directions.

Look at the king:

                if (i + 1 < 8)
                    board_aux[i + 1][j] = '1';
                if (i - 1 > -1)
                    board_aux[i - 1][j] = '1';
                if (j + 1 < 8)
                    board_aux[i][j + 1] = '1';
                if (j - 1 > -1)
                    board_aux[i][j - 1] = '1';
                if (i + 1 < 8 and j + 1 < 8)
                    board_aux[i + 1][j + 1] = '1';
                if (i + 1 < 8 and j - 1 > -1)
                    board_aux[i + 1][j - 1] = '1';
                if (i - 1 > -1 and j + 1 < 8)
                    board_aux[i - 1][j + 1] = '1';
                if (i - 1 > -1 and j - 1 > -1)
                    board_aux[i - 1][j - 1] = '1';

I think I would have written it like this.

                for(int deltaX = -1; deltaX <= 1; ++deltaX) {
                    for(int deltay = -1; deltaX <= 1; ++deltaY) {
                        int y = i + deltaY;
                        int x = j + deltaX;
                        if (x >= 0 && x < 8 && y >= 0 && y < 8) {
                            ++board_aux[y][x];
                        }
                    }
                }

Strange to put the range test in two places

            for (int k = i + 1; k < 8; k++, z++)
            {
                if (z == 8 or board[k][z] != '0') break;

Personally I would do the test for z == 8 in the loop test.

            // This is a test for bounds.
            for (int k = i + 1; k < 8 && z < 8; k++, z++)
            {
                // This is a test for the content of the board.
                if (board[k][z] != '0') break;

Bugs

This looks like it has a bug.

            if (i + 2 < 8 and j + 2 < 8)
                board_aux[i + 1][j + 2] = '1';
            if (i + 2 < 8 and j - 2 > -1)
                board_aux[i + 1][j - 2] = '1';
            if (i - 2 > -1 and j + 2 < 8)
                board_aux[i - 1][j + 2] = '1';
            if (i - 2 > -1 and j - 2 > -1)
                board_aux[i - 1][j - 2] = '1';

Checks and access have different values. This is the kind of bug that would not happen if you had used a loop and variable. This is known as a cut and paste bug.