6
\$\begingroup\$

I built a chess game in C++. My main concern is the design. For example, in the board class it has a 2D array of pieces. This array represents the positions of the pieces, but the piece class also has a variable to store its position (used to check the validity of the move sense each piece has a different move capabilities).

main.cpp

#include <iostream>
#include "board.h"
#include "Piece.h"
#include "Presenter.h"
#include "utilities.h"
int main()
{ 

    Board board;
    Presenter presenter(&board);
    presenter.draw();

    std::cout <<endl<< "make a move by writing old then new coordinates of the piece you want to move ex: (0,1) -> (0,5)" << endl;
    int oldX, oldY, newX, newY;
    while (1)
    {
        cin >> oldX >> oldY >> newX >> newY;

        // making sure the numbers are within the board
        if (oldX > 8 || oldY > 8 || newX > 8 || newY > 8)  
        {
            cout << "one or more number is larger than 8" << endl;
            continue;
        }

        oldX--; oldY--; newX--; newY--;

        //making sure the move is valid 
        if (board.move({ oldX, oldY }, { newX, newY }) == invalid)
        {
            cout << "error: you can't move pieces from the other player " << 
             endl;
        }


        presenter.draw();


    }

}

Board.cpp

#include "board.h"
#include <utility>
#include <algorithm>
using namespace std;
Board::Board()
{ 
    active_player = player1;
    // initializing pieces array 
    for (int i = 0; i < 8; i++)
    {
        for (int j = 0; j < 8; j++) {
            if (i < 2)   pieces[i][j] = new Piece({ i, j }, player1);      //the first two rows for player1
            else if (i>5)    pieces[i][j] = new Piece({ i, j }, player2);  //the last two rows for player2
            else pieces[i][j] = new Piece({ i, j }, empty);                                  //all other rows are empty

        }
    }
}

// changes the position of the piece if its a valid move 
move_state Board::move(Coord old_pos, Coord new_pos)
{
    if (pieces[old_pos.x][old_pos.y]->get_owner() != active_player)
        return invalid; 
    else if (pieces[old_pos.x][old_pos.y]->change_position(new_pos) == valid)
    {

        std::swap(pieces[new_pos.x][new_pos.y], pieces[old_pos.x][old_pos.y]);
        active_player = (active_player == player1) ? player2 : player1;  // toggle active_player after each valid move
        return valid;
    }

    else return invalid;
}



Board::~Board()
{
    for (int i = 0; i < 8; i++)
    {
        for (int j = 0; j < 8; j++) {
            free(pieces[i][j]);

        }
    }
}

board.h

#pragma once
#include "Piece.h"
#include "utilities.h"
#include <vector>
using namespace std;
class Board{
    Player active_player;
    Piece *pieces[8][8];

public:
    Board();
    ~Board();
    move_state move(Coord old_pos, Coord new_pos);
    Player get_player(Coord pos)
    {
        if (pieces[pos.x][pos.y] != nullptr)   return pieces[pos.x][pos.y]->get_owner();
        else return empty;
    }
};

piece.h

#pragma once
#include "utilities.h"

class Piece{
 Player owner;
 Coord position;

public :
    Piece(Coord pos, Player _owner) :
        owner(_owner),
        position(pos)
    {

    }
    Player get_owner(){ return owner; }
    move_state change_position(Coord pos)
    {
        position.x = pos.x;
        position.y = pos.y;
        return valid;              // checking is not supported all moves are valid 
    }
};

presenter.cpp

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

using namespace std;



Presenter::~Presenter()
{

}

void Presenter::draw()
{
    cout << "   1  2  3  4  5  6  7  8 ";
    cout << endl << " | ";
    for (int i = 0; i < 8; i++){

        for (int j = 0; j < 8; j++){
            if (board->get_player({ i, j }) == player1) cout << "A" << "  ";
            if (board->get_player({ i, j }) == player2) cout << "B" << "  ";
            if (board->get_player({ i, j }) == empty) cout << "O" << "  ";
        }
        cout << "| "<< i+1 << endl<<" | ";
    }

}

presenter.h

#pragma once
#include "board.h"
class Presenter
{
    Board * board;

public:

    Presenter(Board* b){
        board = b;
    };
    void draw();

    ~Presenter();
};

utilities.h

#pragma once
enum Player
{
    player1,
    player2,
    empty
};


struct Coord
{
    unsigned char x;
    unsigned char y;
};
enum move_state{
    valid,
    invalid
};
\$\endgroup\$
  • 6
    \$\begingroup\$ This doesn’t answer your question, but the #1 advice I can give is to turn Piece *pieces[8][8]; into Piece pieces[8][8];, and remove all references to operator new. A Piece is small, it doesn’t need dynamic allocation. If it were larger or needed dynamic allocation for another reason, you should use a std::unique_ptr to own each piece. Don’t use new unless there are no alternatives. \$\endgroup\$ – Cris Luengo Dec 22 '18 at 22:02
6
\$\begingroup\$

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It is particularly bad to put it into a header file, so please don't do that.

Use the approprate data types

If the Coord values are unsigned char, then the loops that generate coordinates should use that instead. For convenience, and to allow for flexibility, I'd suggest either something like this:

using CoordInt = unsigned char;
struct Coord
{
    CoordInt x;
    CoordInt y;
};

or perhaps better, make Coord a class and do validation of the coordinates in the constructor so that it is not possible to create invalid coordinates.

Do better error checking

If the coordinates passed to Board::move are invalid (i.e. not actually on the board, the program will not work as intended due to undefined behavior. Eliminate that either by doing validation of the coordinates where needed or, as per the previous suggestion, make it impossible for invalid coordinates to be constructed.

Don't mislead the user

The user is told "make a move by writing old then new coordinates of the piece you want to move ex: (0,1) -> (0,5)" but if the user actually enters those zero-based coordinates, the program crashes, because what the program actually expects are 1-based coordinates without commas or parentheses or any other punctuation. This leads directly to the next suggestion.

Use standard nomenclature where it applies

In chess, the standard algebraic notation has been widely used for decades. Experienced chess players will already know that notation, and it won't matter to players who aren't already familiar. Use that notation instead of the number pairs the program currently used.

Don't use std::endl if '\n' will do

Using std::endl emits a \n and flushes the stream. Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.

Use delete instead of free in C++

The Board destructor uses free instead of delete which is an error. C++ uses new and delete. The other suggestions say you shouldn't use free and delete at all, but I understand that your intent is to have Piece be a base class, so this usage is appropriate once you fix this error.

Use const where practical

The current Piece::get_owner() routine does not (and should not) modify the underlying object, and so it should be declared const:

Player get_owner() const { return owner; }

The same is true of Presenter::draw().

Reduce the use of raw pointers where practical

There is no useful thing that a Presenter can do with a Board pointer that is nullptr, so I'd strongly recommend using a std::shared_ptr instead or by having the Board object be a member of the Presenter class.

Let the compiler create default destructor

The compiler will create a destructor by default which is essentially identical to what you've got for the Presenter class, so you can simply omit both the declaration and implementation from your code.

Use consistent file names

Is it board.h or Board.h? Presenter.h or presenter.h? It's important to be consistent because consistency aids reader comprehension.

Rethink the class design

If we think about a future version of this program that actually has rooks, knights, etc. What will each piece need to determine whether a move is valid? The current interface only passes the proposed new coordinates to change_position. However, this is not enough information. A bishop, for instance, cannot jump over other pieces, so determining a valid move will require examining all of the points between the current and proposed position as well. This information more properly belongs to the Board class, so I would recommend passing a Board reference to the Piece to allow it to check for move validity. Also, for some moves, such as castling, or capturing a pawn en passant, two pieces are involved and not only their positions but something about their history must be known. For all of these reasons, I'd suggest that only the Board should know about positions, and to eliminate the Coord member from each Piece and that any move validity checking should pass not only the proposed Coord but the Board and current Coord as well.

\$\endgroup\$
3
\$\begingroup\$

@CrisLuengo's advice is valid. Past that, even if you were to keep your usage of new, there's a very important rule to follow: never mix C and C++ dynamic memory allocation. You've used new and then free, when you should be using new and then delete. Depending on your C libraries, compiler and system, this kind of mixing can do very bad things, potentially stack smashing, etc.

\$\endgroup\$
3
\$\begingroup\$
  • Consistency helps other people read, understand, and trust your code. This includes:
    • consistent whitespace (e.g. leaving an empty line after #includes),
    • capitalization of file names (board.h vs Presenter.h),
    • variable and type naming style (Coord vs move_state),
    • putting all function definitions in the .cpp files, instead of just some of them.
  • Don't use using namespace std;.
  • Use enum class instead of plain enum for type-safety.

  • Variables should be declared as close to the point of use as practical (oldX, oldY, newX, newY should be inside the loop in main).
  • Validate input thoroughly. There's no guarantee that what the user enters is an int, so we have to check that reading from cin worked each time. If it didn't we then have to clear the error flags on the stream, and ignore the invalid input:

    if (std::cin.fail())
    {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    }
    

    It would therefore be better to abstract the request for user input to a separate function with a signature like: bool readCoord(Coord& value);

  • While we check that the input values are not too high (e.g. oldX < 8), we also have to check that they are not too low (if the user enters -1). Which leads on to...

  • Use unsigned types where numbers should not be negative (e.g. for indexing into arrays).
  • We could make this neater by using the Coord class and adding a new function to the board: if (!(board.InBounds(old) && board.InBounds(new))) ....
  • This line oldX--; oldY--; newX--; newY--; indicates that we're using 1-based indexing, but the "...of the piece you want to move ex: (0,1) -> (0,5)" implies 0-based indexing. Note that for 1-based indexing, we also need to check that we don't have an index of 0, even if we're using an unsigned index type.
  • Nitpick: use while (true) instead of while (1). It's a more direct statement of intent, since the latter is shorthand for while (1 != 0).

  • Don't duplicate the position inside the Piece class. Checking that the move is valid should be done by the board (or better by a separate Rules class).

  • The dynamic allocation of pieces might be understandable if we were using a nullptr for empty squares, but without that there's no advantage over storing all the pieces by value (and all the disadvantages pointed out by Reinderien).

  • It might be better to store pieces in an array of std::optional<Piece>, instead of having a special empty value in your Player enum for pieces that don't actually exist.

  • Presenter should probably store a const Board& instead of a Board*. This means Presenter can never be given a nullptr, and it shows that Presenter never changes the board.

  • Board::get_player can then be a const function: Player get_player(Coord pos) const;, since it doesn't alter the data members of the board class.

  • Presenter can use the member initializer list to initialize the board variable (like the Board class does). Note that initializer lists specifically allow using the same name for function arguments as used for the member variable.

  • Since the Presenter destructor does nothing, we don't need to define it ourselves.

\$\endgroup\$
1
\$\begingroup\$

Pass by reference?

Not everyone agrees, however, references ain't allowed to be nullptr. If you don't expect a nullptr, pass by reference instead of pointer.

Presenter presenter(board);

Initialize your variables

It's always a good idea to initialize your variables, even when replacing the values with cin. Cin leaves the value unmodified, when entering invalid data. So you have undefined behavior in your program.

So either initialize your values to an invalid value OR

Check on input failure

if (!cin >> oldX)
    // Handle error

Check boundaries

As indicated entering a b c d causes problems, entering 0 1 2 3 does as well as it will result in out of bounds access.

Use compound initialization

If things are good they deserve a mention as well. Calling the move with pairs clearly indicates for the caller how it's grouped:

if (board.move({ oldX, oldY }, { newX, newY })

Use standard types

  if (board.move({ oldX, oldY }, { newX, newY }) == invalid)

Why return a special value, when you only check on failure? A bool will work as well.

std::unique_ptr

 if (i < 2)   pieces[i][j] = new Piece({ i, j }, player1);

From C++14 on, using std::make_unique<Piece>(...) is recommended. Storing the value in a unique_ptr indicates ownership clearly, while preventing memory-leak.

Constants

 for (int i = 0; i < 8; i++)

8 is a really nice number, putting it in a constant allows you to give it a name.

Check on pointers

 if (pieces[old_pos.x][old_pos.y]->get_owner() != active_player)

You haven't checked your pointer on null. Again you assume valid input.

Const?

If you don't expect changes to an instance, make it const. If catches simple mistakes and clearly sets expectations.

 Player get_player(Coord pos) const

Initializer list

Presenter(Board& b) : board(b) {
    board = b;
 };

Scoped enumerations

enum class Player { ... };
\$\endgroup\$

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.