Yet another piece of Chess software

Question

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

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 <= 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;
        }
    }

    for (unsigned char it = 0; it <= 1; it++)
    {
        for (signed char xy = (it==0 ? piece_pos.x_pos:piece_pos.y_pos) - 1; xy >= 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 < 8;i++)
    {
        co_ord temp (piece_pos.x_pos + x_offset[i],piece_pos.y_pos + y_offset[i]); //offsets calculated

        if (temp.inRange())
            if (isAvailableSquare(temp,brd.getSquare(piece_pos).getSqrType()))
                tempmoves.append(temp);
    }
}

void Game::kingMoves(const co_ord& piece_pos)
{
    co_ord origin(piece_pos.x_pos - 1, piece_pos.y_pos - 1);
    for (signed char x= origin.x_pos; x < origin.x_pos + 3; x++)
    {
        for (signed char y= origin.y_pos; y < origin.y_pos + 3; y++)
        {
            co_ord temp (x,y);
            if (temp == piece_pos || !temp.inRange())
                continue;

            if (isAvailableSquare(temp,brd.getSquare(piece_pos).getSqrType()))                  
                tempmoves.append(temp);
        }
    }
}

void Game::bishopMoves(const co_ord& piece_pos)
{
    for (signed char it = -1; it < 2; it=it+2)
    {
        for (signed char x = piece_pos.x_pos + it, y = piece_pos.y_pos + it; co_ord (x,y).inRange();x=x + it,y=y +it)
        {
            co_ord temp (x,y);

            if (isAvailableSquare(temp,brd.getSquare(piece_pos).getSqrType()))
                tempmoves.append(temp);

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

    for (signed char it = 0; it < 2; it++)
    {
        signed offset_x = (it == 0 ? 1:-1), offset_y = (it == 0 ? -1: 1);

        for (signed char x = piece_pos.x_pos + offset_x, y = piece_pos.y_pos + offset_y; co_ord (x,y).inRange(); x= x+ offset_x, y = y + offset_y)
        {
            co_ord temp (x,y);

            if (isAvailableSquare(temp,brd.getSquare(piece_pos).getSqrType()))
                tempmoves.append(temp);

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

void Game::queenMoves(const co_ord& piece_pos)
{
    castleMoves(piece_pos);
    bishopMoves(piece_pos);
}

void Game::pawnMoves(const co_ord& piece_pos)
{
    Squaretype colour = brd.getSquare(piece_pos).getSqrType();
    signed char x_offset = (colour == WHITE ? 1:-1);

    //FORWARD
    co_ord temp_x1(piece_pos.x_pos + x_offset,piece_pos.y_pos); // 1 place 'forward'

    if (temp_x1.inRange() && brd.getSquare(temp_x1).isEmpty())      
    {
        tempmoves.append(temp_x1);
        co_ord temp_x2(piece_pos.x_pos + x_offset + x_offset, piece_pos.y_pos);

        if ((piece_pos.x_pos == 1 && colour == WHITE) || (piece_pos.x_pos == 6 && colour == BLACK)) //double move allowed...
            if (brd.getSquare(temp_x2).isEmpty()) //as long as that space is empty
                tempmoves.append(temp_x2);
    }

    //CAPTURE MOVES
    for (signed char y = piece_pos.y_pos - 1; y <= piece_pos.y_pos +1; y=y+2)
    {
        co_ord temp_y (piece_pos.x_pos + x_offset,y);
        if (temp_y.inRange() && colour != brd.getSquare(temp_y).getSqrType() && !brd.getSquare(temp_y).isEmpty())
            tempmoves.append(temp_y);
    }

}

//generic functions

bool Game::isAvailableSquare (const co_ord& pos,const Squaretype& piece_type)
{

    Squaretype pos_type = brd.getSquare(pos).getSqrType();
    Squareoccupier pos_occ = brd.getSquare(pos).getSqrOccupier();

    if (pos_type == TYPE_EMPTY)
        return true;

    if (pos_type == piece_type)  //cannot take own piece
        return false;

    return true;    //can take other piece (note: includes king)
}

bool Game::inCheck(const Squaretype& colour)
{
    tempmoves.reset();
    co_ord king_pos = brd.findKing(colour);

    for (unsigned char x = 0; x <= MAX_RANGE;x++)
    {
        for (unsigned char y = 0; y <= MAX_RANGE; y++)
        {
            co_ord temp (x,y);
            if (brd.getSquare(temp).getSqrType() != colour && !brd.getSquare(temp).isEmpty()) //iterate over opposing colour piece
            {
                generatePieceMoves(temp); //rather than generateLegalMoves as we need to include opp king pos

                if (tempmoves.match(king_pos))  //check to see whether our king is a target
                {
                    tempmoves.reset();
                    return true;
                }

                tempmoves.reset();  //clear for next round      
            }
        }
    }
    return false;
}

bool Game::inCheckmate(const Squaretype& colour)
{
    if (!inCheck(colour))
        return false;   //cannot be in checkmate if not in check

    return (hasNoLegalMoves(colour));

}

bool Game::inStalemate(const Squaretype& colour)
{
    if (inCheck(colour))
        return false;   //cannot be in stalemate if in check

    return (hasNoLegalMoves(colour));


}

bool Game::hasNoLegalMoves (const Squaretype& colour)
{

    for (unsigned char x = 0; x <= MAX_RANGE; x++)
    {
        for (unsigned char y = 0; y <= MAX_RANGE; y++)
        {
            co_ord temp (x,y);
            if (brd.getSquare(temp).getSqrType() != colour) //iterate over own pieces only
                continue;

            generateLegalMoves (temp);

            for (unsigned char it = 0; it < tempmoves.getSize(); it++) //iterate over possible moves
            {
                Game future = *this;    //copy current game

                future.makeMove(temp,tempmoves.getCo_ord(it));

                if (!future.inCheck(colour)) //check relieved -> 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)
        knightMoves(piece_pos);

    if (piece_occ == WHITE_BISHOP || piece_occ == BLACK_BISHOP)
        bishopMoves(piece_pos);

    if (piece_occ == WHITE_QUEEN || piece_occ == BLACK_QUEEN)
            queenMoves(piece_pos);

    if (piece_occ == WHITE_KING || piece_occ == BLACK_KING)
        kingMoves(piece_pos);

}

void Game::generateLegalMoves(const co_ord& piece_pos) //same as generatePieceMoves but removes opp king positions
{   
    generatePieceMoves (piece_pos);

    for (unsigned char it = 0; it < tempmoves.getSize();it++)
    {
        Squareoccupier temp_piece = brd.getSquare (tempmoves.getCo_ord(it)).getSqrOccupier();
        if (temp_piece == WHITE_KING || temp_piece == BLACK_KING)  //remove from tempmoves if a king
            tempmoves.remove (it);
    }
}

//print

void Game::printTempmoves()
{
    for (unsigned char i=0; i< tempmoves.getSize();i++)
        std::cout <<(int) tempmoves.getCo_ord(i).x_pos << ","<<  (int) tempmoves.getCo_ord(i).y_pos <<std::endl;
}

void Game::printBoard()
{
    std::cout << " ";
    for (char pos=0;pos<8;pos++)
        std::cout <<(int) pos;

    std::cout <<std::endl;

    for (char posx=0;posx<8;posx++)
    {
        std::cout<<(int)posx;
        for (char posy=0;posy<8;posy++)
            std::cout << (char) brd.getSquare(co_ord(posx,posy)).getSqrOccupier();

        std::cout<<std::endl;

    }
}

//gameplay functions

void Game::makeMove (const co_ord& from, const co_ord& to)
{
    brd.setSquare(brd.getSquare(from).getSqrOccupier(),to);
    brd.setSquare(EMPTY,from);
}

Game.h

#ifndef GAME_H
#define GAME_H

#include "Board.h"

class Game
{
    co_ord_list tempmoves;  
    bool isAvailableSquare (const co_ord&,const Squaretype&); //checks square to see if empty or can be taken

    Board brd;
    public:
    Game();

    //generate possible moves by piece
    void castleMoves (const co_ord&);
    void knightMoves (const co_ord&);
    void kingMoves (const co_ord&);
    void bishopMoves (const co_ord&);
    void queenMoves (const co_ord&);
    void pawnMoves (const co_ord&);


    void generatePieceMoves(const co_ord&);     // apply move generation for that square (for check)
    void generateLegalMoves(const co_ord&);     // as with generatePieceMoves but removes opp king positions

    //check etc

    bool inCheck(const Squaretype&);
    bool inCheckmate (const Squaretype&);
    bool inStalemate(const Squaretype&);
    bool hasNoLegalMoves(const Squaretype&);        //used in inCheckmate and inStalemate

    //print functions

    void printTempmoves();
    void printBoard();

    //gameplay functions
    void makeMove (const co_ord& from, const co_ord& to);


};

#endif

Board.cpp

#include "Board.h"
#include <iostream>


void Board::setSquare (const Squareoccupier &piece_in, const co_ord &dest_pos)
{
    board[dest_pos.x_pos][dest_pos.y_pos].setSqrOccupier(piece_in);
}

Square Board::getSquare (const co_ord &get_pos)
{
    return board[get_pos.x_pos][get_pos.y_pos];
}

void Board::initialiseBoard()
{

//everything to empty

    for (char posx=0;posx<8;posx++)
        for (char posy=0;posy<8;posy++)
            setSquare(EMPTY,co_ord(posx,posy));

//whites

    for (char pos=0;pos<8;pos++)
        setSquare(WHITE_PAWN,co_ord(1,pos));


    setSquare(WHITE_CASTLE,co_ord(0,0));
    setSquare(WHITE_CASTLE,co_ord(0,7));
    setSquare(WHITE_KNIGHT,co_ord(0,1));
    setSquare(WHITE_KNIGHT,co_ord(0,6));
    setSquare(WHITE_BISHOP,co_ord(0,2));
    setSquare(WHITE_BISHOP,co_ord(0,5));
    setSquare(WHITE_KING,co_ord(0,4));
    setSquare(WHITE_QUEEN,co_ord(0,3));

//blacks
    for (char pos=0;pos<8;pos++)
        setSquare(BLACK_PAWN,co_ord(6,pos));

    setSquare(BLACK_CASTLE,co_ord(7,0));
    setSquare(BLACK_CASTLE,co_ord(7,7));
    setSquare(BLACK_KNIGHT,co_ord(7,1));
    setSquare(BLACK_KNIGHT,co_ord(7,6));
    setSquare(BLACK_BISHOP,co_ord(7,2));
    setSquare(BLACK_BISHOP,co_ord(7,5));
    setSquare(BLACK_KING,co_ord(7,4));
    setSquare(BLACK_QUEEN,co_ord(7,3));
}

co_ord Board::findKing(const Squaretype& colour)
{
    for (unsigned char x = 0; x <= MAX_RANGE; x++)
        for (unsigned char y = 0; y <= MAX_RANGE; y++)
            if (getSquare(co_ord(x,y)).getSqrOccupier() == (colour == WHITE ? WHITE_KING:BLACK_KING))
                return co_ord (x,y);
}

Board.h

#ifndef BOARD_H
#define BOARD_H

#include "Square.h"

class Board
{
    Square board[8][8];

    public:

    //access and initialisation
    void setSquare (const Squareoccupier&,const co_ord&);
    Square getSquare (const co_ord&);
    void initialiseBoard();


    //no copy constructor required

    co_ord findKing (const Squaretype&);


};

#endif

Square.cpp

#include "Square.h"

Squareoccupier Square::getSqrOccupier()
{
    return sqr;
}


Squaretype Square::getSqrType()
{
    if (sqr == EMPTY)
        return TYPE_EMPTY;

    if (sqr >= 'A' && sqr <= 'Z')   //ERROR
        return WHITE;

    if (sqr >= 'a' && sqr <= 'z')
        return BLACK;
}

void Square::setSqrOccupier(const Squareoccupier& sqr_in)
{
    sqr = sqr_in;
}

bool Square::isEmpty()
{
    if (sqr == EMPTY) 
        return true;

    return false;
}

Square.h

#ifndef SQUARE_H
#define SQUARE_H

#include "Chesstypes.h"

class Square
{
    Squareoccupier sqr;

    public:

    Squareoccupier getSqrOccupier();
    Squaretype getSqrType();
    void setSqrOccupier(const Squareoccupier&);

    bool isEmpty();



};

#endif

Chesstypes.cpp

#include "Chesstypes.h"

//CO_ORD 

co_ord::co_ord (signed char x_in, signed char y_in)
{
    x_pos=x_in,y_pos=y_in;

}

co_ord::co_ord ()
{


}

bool co_ord::operator==(const co_ord &co_ord_compare) const
{
    if (co_ord_compare.x_pos == x_pos && co_ord_compare.y_pos == y_pos)
        return true;

    return false;

}

bool co_ord::inRange()
{
    return (x_pos <= MAX_RANGE && x_pos >= MIN_RANGE && y_pos <= MAX_RANGE && y_pos >= MIN_RANGE);
}

//CO_ORD_LIST

co_ord_list::co_ord_list()
{

    current_size = 0;

}

void co_ord_list::append (co_ord co_in)
{
    list[getSize()] = co_in;
    current_size++;

}

unsigned char co_ord_list::getSize() const
{

    return current_size;
}

co_ord co_ord_list::getCo_ord(unsigned char pos)
{
    return list[pos];

}

void co_ord_list::reset()
{
    current_size = 0;

}

bool co_ord_list::match(const co_ord& input) const
{
    for (unsigned char i = 0; i < getSize(); i++)
        if (input == list[i])
            return true;

    return false;
}

void co_ord_list::remove(unsigned char& i) 
{

    current_size--;

    if (i == (getSize()-1)) //last element
        return;

    for (unsigned char it = i; it < getSize(); it++) //shift everything back a place
        list [it] = list[it + 1];   

}

Chesstypes.h

#ifndef CHESSTYPES_H
#define CHESSTYPES_H

enum Ranges
{
    MAX_RANGE = 7,
    MIN_RANGE = 0
};

enum Squareoccupier
{
    EMPTY = ' ',

    WHITE_PAWN = 'P',
    WHITE_CASTLE = 'C',
    WHITE_KNIGHT = 'N',
    WHITE_BISHOP = 'B',
    WHITE_QUEEN = 'Q',
    WHITE_KING = 'K',


    BLACK_PAWN = 'p',
    BLACK_CASTLE = 'c',
    BLACK_KNIGHT = 'n',
    BLACK_BISHOP = 'b',
    BLACK_QUEEN = 'q',
    BLACK_KING = 'k'


};

enum Squaretype
{
    WHITE = 11,
    BLACK = 12,
    TYPE_EMPTY = 13

};

struct co_ord
{
    signed char x_pos,y_pos;

    co_ord (signed char a,signed char b);
    co_ord ();
    bool operator==(const co_ord &a) const;
    bool inRange();

};

struct co_ord_list
{


    co_ord_list();
    void append (co_ord);
    unsigned char getSize() const;
    co_ord getCo_ord (unsigned char);
    void reset();
    bool match (const co_ord&) const;
    void remove (unsigned char& i);

    private:
    unsigned char current_size;
    co_ord list[64];

};


#endif

Answer 1

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.

Answer 2

A few additional points:

• You have a lot of variables named "temp". This name provides little or no clue to what the actual variable contains; I'd suggest using something more meaningful for this.
• Your board's methods that have co_ord parameters take a reference to them, but co_ord is a type that is intended to contain a value. In most cases, it is better to allow the co_ord instance to be copied, so you should usually just pass instances directly. Using references is often intended as an optimization, but in many cases (particularly where, as in this case, the object is small) it doesn't actually help, and can in fact hinder (by preventing certain optimizations that can be performed by the compiler when it knows two objects are not references to each other, for example).
• You have inconsistent capitalization for your type names. Board, Game and Square are all in one standard, but co_ord is using a different one (should probably be Coord), and your enums like Squareoccupier and Squaretype would probably be easier to read if you capitalize the start of each word rather than just the first, i.e. SquareOccupier and SquareType.
• More a preference than anything else, but I don't like the naming in your Square methods that contain Sqr -- the fact that they operate on a square is implied by the context, so you don't need to mention it, and even if it wasn't clear using the abbreviation is just confusing.
• Again, just a preference, but I would implement an operator [] overload on Board for getting the contents of a square, as I think it would make code using the Board clearer. But I understand a lot of people don't like this approach, so I just put it out as a point to consider rather than a criticism. :)