Yet another piece of Chess software

I've written a piece of chess software which does some validation checks (check, checkmate etc). I've not implemented some of the more exotic Chess moves (e.g. rooking, en passant, promotion).

I've written the code for a platform where the STL is not available. Thus, std::vectorhas been deliberately avoided (I'm aware that there are some calls to iostream library).

I'd be very grateful if someone could review my code from a design and stylistic perspective (not so much from an algorithmic perspective; I'm aware there are much more efficient implementations).

Game.cpp

```
#include "Game.h"
#include

Game::Game()
{
brd.initialiseBoard();
}

//piece validity

void Game::castleMoves(const co_ord& piece_pos)
{
Squaretype piece_type = brd.getSquare(piece_pos).getSqrType();

for (unsigned char it = 0; it = MIN_RANGE; xy--)
{
co_ord temp (it == 0 ? xy:piece_pos.x_pos,it == 0 ? piece_pos.y_pos:xy);
if (isAvailableSquare(temp,piece_type))
tempmoves.append(temp);

if (!brd.getSquare(temp).isEmpty())
break;
}
}
}

void Game::knightMoves(const co_ord& piece_pos)
{
//OFFSETS
signed char x_offset [8] = {-2,-2,-1,-1,1,1,2,2};
signed char y_offset [8] = {1,-1,-2,2,-2,2,-1,1};

for (unsigned char i=0; i legal moves left
{
tempmoves.reset();
return false;
}
}
tempmoves.reset();
}
}
return true; //no moves

}

void Game::generatePieceMoves(const co_ord& piece_pos)
{
Squareoccupier piece_occ = brd.getSquare(piece_pos).getSqrOccupier();

if (piece_occ == EMPTY)
return;

if (piece_occ == WHITE_PAWN || piece_occ == BLACK_PAWN)
pawnMoves(piece_pos);

if (piece_occ == WHITE_CASTLE || piece_occ == BLACK_CASTLE)
castleMoves(piece_pos);

if (piece_occ == WHITE_KNIGHT || piece_occ == BLACK_KNIGHT)
kni

for (unsigned char it = 0; it <= 1; it++)
{
    for (signed char xy = (it==0 ? piece_pos.x_pos:piece_pos.y_pos) + 1; xy <= MAX_RANGE; xy++)
    {
        co_ord temp (it == 0 ? xy:piece_pos.x_pos,it == 0 ? piece_pos.y_pos:xy);
        if (isAvailableSquare(temp,piece_type))
            tempmoves.append(temp);         

        if (!brd.getSquare(temp).isEmpty())
            break;
    }
}

This single piece of code has several problems in it. First, we'll take a look at the outer loop.

for (unsigned char it = 0; it <= 1; it++)

You start at 0 and go until it == 1. Essentially, this loop iterates twice, once for the x value and once for the y value. This is rather unclear when first reading it, and you should probably extract the loop body out and call it explicitly for the x and y moves.

Why do you use char instead of int? char is for characters and int is for integers. Also, why the use of unsigned? Using unsigned for just any value that is never expected to be negative is considered a bad practice by the answers on an SO post about the topic:


Unsigned values are great and very useful, but NOT for representing container size or for indexes; for size and index regular signed integers work much better because the semantic is what you would expect.

Please space your operators out evenly and consistently. At first glance, I though the : operator was part of the expression because it did not have spaces around it. You usually use spaces around the == operator, but you left them out in once place.

Please check your naming. What does it represent? xy represents two different values, depending on the value of it, which no variable should ever do and indicates a problem both with naming and with usage. What type is co_ord? Is it a misspelling of coord?, or is it two partial words joined with a _? board is more clear than brd, and C++ does not limit variable name length, nor is it faster with shorter variable names.

Please use your braces around one-line if statements. It will help prevent bugs if you ever need to add statements to the body, and helps readers see what belongs to what. It does help that your indentation is consistent here.

I don't like the way you do tempmoves.append(temp) here. It would be better to create a list of moves within the function and return it. If you really need a concatenated list of all moves, then do that in the controller function that calls each of these functions. Something like:

if (piece_occ == WHITE_CASTLE || piece_occ == BLACK_CASTLE)
    tempmoves.addRange(Game::castleMoves(...));
// other piece moves here...

instead of:

if (piece_occ == WHITE_CASTLE || piece_occ == BLACK_CASTLE)
    castleMoves(piece_pos);

Also, did I mention that these should be called "Rooks", not "Castles"?

Looking further into the Game::castleMoves function, you have a lot of duplication in those loops. I would recommend first, just iterating from the smallest value to the largest (that's one use for Math.Min()) to combine the two outer loop blocks, and second, writing out the x and y coordinate loops separately so it is more clear what they do. This will lead to more duplication, but you can split the duplicated logic into reusable helper functions.

StackExchange Code Review Q#150692, answer score: 13