Some problems with an abstract class

Hey guys, I am having a few problems with this abstract class I am working on.

We are building a chess game in class, and using inheritance for the different pieces.

So my base class is simply called PIECE, and there are six classes that inherit from it: PAWN, ROOK, KNIGHT, BISHOP, QUEEN, and KING.

Well, most of the functionality of all the pieces is the exact same, so most of the functionality lies in the PIECE class itself. However, I have made the destructor virtual, as well as a function called GetPossibleMoves and also WriteToXML.

GetPossibleMoves is virtual obviously because every type of piece has a different set of possible moves. WriteToXML is virtual because...well, for similar reasons.

Anyways, the problem is this:

For the most part in my program I refer to all the pieces as PIECE in general. Very very rarely do I actually refer to them by their child class names. My desire is that when I call the GetPossibleMoves function from the base class of PIECE, that it automatically calls the correct child version of it. For all intents and purposes it should be doing so...however...it's not...

I orginally had GetPossibleMoves defined as basically an empty function on the PIECE level, and then on its childrens' levels, it was defined for each piece to get the moves of that piece.

I noticed that whenever I called GetPossibleMoves, it was returning to me a set of 0's...which means that no moves are possible....well...the only way that is possible is if it is calling the parent GetPossibleMoves, and not the child version.

So I decided to change the parent function from virtually an empty function to this:

virtual bitset <64> GetPossibleMoves ( ) = 0;

This brings up a different problem: It makes it a pure abstract class. Pure abstract classes cannot be instantiated or worked with, except with pointers. Well, right now my program isn't using pointers, and it would take a great deal of work to make it use pointers.....although it is one option.

For reference, I will post the code of where the pieces are being created, and then where they are being used.

The piece creation function:

Code:

---

        /* CreatePiece() - creates a piece and places it on the specified position.
                This function returns the piece's assigned id number. */
        int BOARD::CreatePiece ( int type, int owner, int col, int row )
        {
                /* The tasks of this function include:
                       
                                1.  Create a PIECE object for the new piece.  Insert it
                                        into the chessPieces map with a new ID #.
                                2.  Place the piece on the chessBoard object at the
                                        appropriate position.
                                3.  Update the AllPieces, WhitePieces, and BlackPieces
                                        bitboards appropriately.                       
                */
               
                PIECE newPiece;
               
                switch ( type )
                {
                        case 1:
                        case 7:
                                newPiece = ROOK();
                                break;
                        case 2:
                        case 8:
                                newPiece = KNIGHT();
                                break;
                        case 3:
                        case 9:
                                newPiece = BISHOP();
                                break;
                        case 4:
                        case 10:
                                newPiece = QUEEN();
                                break;
                        case 5:
                        case 11:
                                newPiece = KING();
                                break;
                        case 6:
                        case 12:
                        default:
                                newPiece = PAWN();
                                break;
                }

                newPiece.SetPosition( col, row );
                newPiece.SetOwner( owner );
                newPiece.SetType ( type );
               
                //The ID number is set
                newPiece.SetID( (++PrevPieceID) );
               
                //Place the piece on the chess board
                chessBoard[col][row] = newPiece.GetID();
               
                //Place the piece in the chess piece map
                chessPieces [ newPiece.GetID() ] = newPiece;
               
                //Place the piece in the appropriate bitboards.
                AllPieces [ row * 010 + col ] = true;
                if ( owner == WHITE_TEAM )
                        WhitePieces [ row * 010 + col ] = true;
                else BlackPieces [ row * 010 + col ] = true;               
               
                return newPiece.GetID(); //return the new piece's ID       
        }

---

The on-mouse-click event handler, which references pieces and asks them for their possible moves:

Code:

---

void Chess::on_mouse_button_pressed(int c, int r, int type, int button)
{       
        cout << "Mouse button pressed..." << endl;

        BOARD gameBoard = gameManager.GetGameBoard();
        PIECE pieceSelected;

        pieceSelected = gameBoard.GetPiece ( c, r );
        bitset<64> pMoves = pieceSelected.GetPossibleMoves();
       
        cout << "Piece selected type: " << pieceSelected.GetType() << endl;
        cout << "Possible moves bitset: " << pMoves << endl;

        for ( int row = 0; row < 8; row++ )
        {
                for ( int col = 0; col < 8; col++ )
                {
                        if ( pMoves[ row * 8 + col ] == true )       
                                highlight_square ( c, r, BLUE_SQUARE );
                }
        }
}

---

You have to use pointers or references if you want the virtual mechanism to do its job.

>> newPiece = ROOK();
What you are doing here and with the other types is called slicing. You are copying a derived piece to a base class piece which only saves the base class information. To use polymorphism, newPiece needs to be a PIECE* or PIECE&.

For a chess game, you know exactly how many pieces there will be on the board, so you have some options on how to create them. The normal method of creating objects for use with polymorphism is to use new to dynamically allocate the memory for the piece and save that pointer somewhere (e.g. in your chessPieces vector/array). When the board is destroyed, you should loop through and delete all the pieces. You can also switch to a boost::ptr_vector<PIECE> or vector<shared_ptr<PIECE> > which will do the cleanup automatically. Another option is to include statically creating the pieces (since you know exactly how many there will be) and storing pointers without having to remember to delete them.

Regardless, you will have to be doing all of your function calling via pointers or references. I would change all functions that take a PIECE and make them take a const PIECE& instead (assuming your code is const correct, otherwise just use a PIECE&). All code that accesses a PIECE directly from the chessPieces container would treat it as a pointer, otherwise you would use references. This will minimize the code that needs to change, as references use the same syntax as regular objects.

I would change GetPiece to return a reference or const reference, so your code that uses it would look like this:

Code:

---

PIECE& pieceSelected = gameBoard.GetPiece ( c, r );

---

This way, you won't be copying and slicing, and the virtual function mechanism will still work. If GetPiece can return an invalid piece, you might have to use a pointer there instead.

thanks Daved. I was hoping to avoid having to deal with memory management on this specific project, but I guess it builds character :)

[EDIT AGAIN]

I am editing this post a bit based on some new stuff that has come up. I discovered that a part of my problem was in the copy constructor of my BOARD class. I was passing the BOARD around to various parts of the program, by value, and therefore the copy constructor and the destructor were being called often. Well, whenever the destructor was being called, it was killing my piece information. I was hesitant to create new copies of the pieces every time I wanted to pass the BOARD around, so I decided to just pass the game board by reference using pointers and stuff, so that way the destructor wouldnt get called until the very end of the game when I want it to be called. That way there is no bad stuff happening to my piece data. So that problem is solved. I clicked on the pieces and they began to work for the most part, EXCEPT for bishops. An odd problem, that only bishops wouldn't work?

[/END 2ND EDIT]

The pieces are stored in a map like thus:

Code:

---

map < int, PIECE * > chessPieces;

---

The segmentation fault is happening in the following function. It is the same function as it was happening before, and in fact it is happening on the same line of code. The difference is that the fault is isolated to only bishops now. That code follows:

Code:

---

void Chess::on_mouse_button_pressed(int c, int r, int type, int button)
{       
        cout << "Mouse button pressed..." << endl;

        BOARD * gameBoard = gameManager.GetGameBoard();
        PIECE *pieceSelected;

        pieceSelected = gameBoard->GetPiece ( c, r );

        if ( !pieceSelected )
        {
                cout << "NULL square selected!" << endl;
                return;
        }
        else
        {
                pieceSelected->Test(cout);
        }

        bitset<64> pMoves = pieceSelected->GetPossibleMoves();
       
        cout << "Piece selected type: " << pieceSelected->GetType() << endl;
        cout << "Possible moves bitset: " << pMoves << endl;

        for ( int row = 0; row < 8; row++ )
        {
                for ( int col = 0; col < 8; col++ )
                {
                        if ( pMoves[ row * 8 + col ] == true )       
                                highlight_square ( c, r, BLUE_SQUARE );
                }
        }

        gameBoard = NULL;
}

---

The code is faulting on this line:

Code:

---

        bitset<64> pMoves = pieceSelected->GetPossibleMoves();

---

The piece creation function looks thus:

Code:

---

        /* CreatePiece() - creates a piece and places it on the specified position.
                This function returns the piece's assigned id number. */
        int BOARD::CreatePiece ( int type, int owner, int col, int row )
        {
                /* The tasks of this function include:
                       
                                1.  Create a PIECE object for the new piece.  Insert it
                                        into the chessPieces map with a new ID #.
                                2.  Place the piece on the chessBoard object at the
                                        appropriate position.
                                3.  Update the AllPieces, WhitePieces, and BlackPieces
                                        bitboards appropriately.                       
                */
               
                PIECE *newPiece;
               
                switch ( type )
                {
                        case 1:
                        case 7:
                                newPiece = new ROOK();
                                break;
                        case 2:
                        case 8:
                                newPiece = new KNIGHT();
                                break;
                        case 3:
                        case 9:
                                newPiece = new BISHOP();
                                break;
                        case 4:
                        case 10:
                                newPiece = new QUEEN();
                                break;
                        case 5:
                        case 11:
                                newPiece = new KING();
                                break;
                        case 6:
                        case 12:
                        default:
                                newPiece = new PAWN();
                                break;
                }

                newPiece->SetPosition( col, row );
                newPiece->SetOwner( owner );
                newPiece->SetType ( type );
               
                //The ID number is set to be the integer value of all pieces
                //currently on the board.  This should be different for every piece
                //when it is set on the board.
                newPiece->SetID( (++PrevPieceID) );
               
                //Place the piece on the chess board
                chessBoard[col][row] = newPiece->GetID();
               
                //Place the piece in the chess piece map
                chessPieces [ newPiece->GetID() ] = newPiece;
               
                //Place the piece in the appropriate bitboards.
                AllPieces [ row * 010 + col ] = true;
                if ( owner == WHITE_TEAM )
                        WhitePieces [ row * 010 + col ] = true;
                else BlackPieces [ row * 010 + col ] = true;               
               
                return newPiece->GetID(); //return the new piece's ID       
        }

---

Those are the pieces of code that are probably the most prone to have pointer errors in them. I am searching for where the error could be, but so far I haven't found it yet, so you guys see anything blatantly obvious or annoyingly subtle that I might be missing?

I thought it might be useful to post this function as well. It is a short but important one:

Code:

---

        /* GetPiece() - returns a pointer to the PIECE at Column and Row */
        PIECE * BOARD::GetPiece ( int Column, int Row )
        {
                if ( chessBoard[Column][Row] == -1 )
                        return NULL;
                else return chessPieces [ chessBoard[Column][Row] ];       
        }

---

huh...seems to oddly be working now...oh well :)

Since you really don't want to be copying the BOARD around, you should make the copy constructor and copy assignment operator private (you don't have to implement them, just declare them in the private section of the class). That way, if you ever accidentally make a typo and copy the BOARD, the compiler will flag the error immediately and you can fix it instead of having it cause a crash later do to the deletion of the pieces.

You also should make PIECE::GetPossibleMoves pure virtual. That way, your compiler will prevent you from instantiating PIECEs that aren't actually derived classes.

Rule: Make as many possible error situations as possible fail at compilation. The compiler is your best friend when it comes to ensure program correctness, so work with it, not against it. Use non-copyable objects and let the compiler ensure you don't copy them. Use abstract classes and let the compiler ensure you don't instantiate them. Use smart pointers and let the compiler ensure the memory is freed.

In fact, that is the same reason you should consider making your program const-correct. While it may be a lot of effort, it is yet another way to let the compiler find your errors for you.

And why you should really think hard about every cast you do.