Knight Tour Index and Moving Algorithm Bugs

Question

The goal of this program is for the knight to move around the chest board and only touching each spot once.

Each spot is initialized and set to zero by default. As the knight moves, each spot the knight touches should correspond with the number of moves taken to reach that point.

However, I am having quite a few problems

1) My Knight is moving around the board and going either out of bound of the multidimensional chess board array or manipulates the movement arrays (horizontal[] and vertical[])

2) The conditions of my boolean functions MoveOnBoard && MoveHasNotBeenMade are that if the next possible move is between the exisiting rows and columns also if the spot being moved to has a value of 0(meaning it has yet to be moved to). However, both of these conditions seem to be ignored.

How would I go about fixing this?

Thank you in advance!

Here's the code below

using namespace std;
#include <iostream>
#include <array>

void DefinedMoveSet();
void RenderBoard();
void MoveKnight(int& moveChoice, int& numberOfMovesMade);
void PossibleMoves();
bool MoveOnBoard(int& moveChoice);
bool MoveHasNotBeenMade(int& moveChoice);

// Two single dimenisional arrays to store move positions for the Knight
// Arrays have yet to be assigned values
int vertical[8], horizontal[8];
int currentRow = 4, currentColumn = 3;

// Initializing an array with the dimension 8 * 8
int chestBoard[8][8] = { 0 };

int main()
{
    DefinedMoveSet();
    PossibleMoves();
    RenderBoard();
    cin.ignore();
    return 0;
}

void RenderBoard()
{
    // The outer loop goes through each row until it reaches 8
    for (int boardRow = 0; boardRow < 8; boardRow++)
    {
        // The inner loop takes in the specific row 
        for (int boardColumn = 0; boardColumn < 8; boardColumn++)
        {
            // Then iterates through the columns of that row until it reaches 8
            // Each index is seperated by a tab escape key shortcut
            cout << chestBoard[boardRow][boardColumn] << "\t";
        }
        // Back to the inner array a new line is printed for the next row
        cout << "\n";
    }
}

void DefinedMoveSet()
{
    // Values for the horizontal array at each index
    horizontal[0] = 2;
    horizontal[1] = 1;
    horizontal[2] = -1;
    horizontal[3] = -2;
    horizontal[4] = -2;
    horizontal[5] = -1;
    horizontal[6] = 1;
    horizontal[7] = 2;

    // Values for the vertical array at each index
    vertical[0] = -1;
    vertical[1] = -2;
    vertical[2] = -2;
    vertical[3] = -1;
    vertical[4] = 1;
    vertical[5] = 2;
    vertical[6] = 2;
    vertical[7] = 1;
}

bool MoveOnBoard(int& moveChoice)
{
    int futureRow = currentRow + vertical[moveChoice];
    int futureColumn = currentColumn + horizontal[moveChoice];

    if ((0 < futureRow) && (0 < futureColumn) && (futureRow < 8) && (futureColumn < 8))
        return true;
}

bool MoveHasNotBeenMade(int& moveChoice)
{
    int futureRow = currentRow + vertical[moveChoice];
    int futureColumn = currentColumn + horizontal[moveChoice];

    if (chestBoard[futureRow][futureColumn] == 0)
        return true;
}

void PossibleMoves()
{
    bool movesStillExist = true;

    int numberOfMovesMade = 1;

    while (numberOfMovesMade < 65 && movesStillExist)
    {
        for (int i = 0; i < 8; i++)
        {
            if (i == 8)
                movesStillExist = false;

            if (MoveOnBoard(i) && MoveHasNotBeenMade(i))
            {
                numberOfMovesMade++;
                MoveKnight(i, numberOfMovesMade);
            }
        }
    }
}

void MoveKnight(int &moveChoice, int &numberOfMovesMade)  
{
    // Takes in the int moveNumber as a parameter
    // MoveNumber(or case) must be between 0 and 7
    // if there is not a case for the value then the knight will not move
    //chestBoard[currentRow][currentColumn] = numberOfMovesMade;
    currentRow += vertical[moveChoice];
    currentColumn += horizontal[moveChoice];
    chestBoard[currentRow][currentColumn] = numberOfMovesMade;
}

What have you tried so far? Used a debugger? Printed out the states of defined and possible moves, that is the content of horizontal and vertical? Also, that is not nice C++. You want to avoid global variables. Why not make this a class, like KnightBoard or whatever? Why do you iterate within PossibleMoves? The name suggests that this only computes which moves are allowed, not that it actually moves. Arrays should be avoided in C++ and vertical and horizontal should not be separated, I'd recommend something like struct Move{ int horizontal, int vertical}; std::vector MoveSet;. — Aziuth, Feb 08 '17 at 11:01
Question, within PossibleMoves, the for loop is not terminated when a legal move has been found. Is this by design? And what is the if(i==8) for? The condition can never be fulfilled, if I don't overlook some side effect on i. In any case, your code is far too tangled, you want some simple, controlled layout. — Aziuth, Feb 08 '17 at 11:06
Don't ignore your compiler warnings. `MoveOnBoard` and `MoveHasNotBeenMade` invoke undefined behavior due to not returning a value for all paths. What happens if the return value isn't `true`? — PaulMcKenzie, Feb 08 '17 at 11:13
@Aziuth Thank you for your response. You pointed out that I was not terminating the loop when a legal move was being made.... No that wasn't by design XD. — Ham, Feb 08 '17 at 11:39
!@Aziuth And yes I know that arrays a not the most suited for this however this is an assignment for one of my classes and it asks for the use of single and 2D arrays. — Ham, Feb 08 '17 at 11:42
@Aziuth Yes, my overall format of my script is crap... I just wanted the damn thing to work. Thank you for your time I will fix it up and post an answer! — Ham, Feb 08 '17 at 11:44

score 1 · Accepted Answer · answered Feb 08 '17 at 11:05

1

in MoveOnBoardand and in MoveHasNotBeenMade instead of

if(...)
  return true;

should be

if(...)
  return true;
return false;

if condtion == false, function returning not void reach end without return statement.

answered Feb 08 '17 at 11:05

Andrew Kashpur

736
5
13

Or even just `return (...)`. Also, in `MoveOnBoard()`, I think the first two tests should be `0 <= future...` instead of `<` since (as far as I can see), zero is a valid row/column. – TripeHound Feb 08 '17 at 11:21
@TripeHound yes! Thank you for noticing my logic error. Changing this helped me fix my program – Ham Feb 08 '17 at 17:40

score 0 · Answer 2 · answered Feb 08 '17 at 17:38

With the advice from the comments I received, I was able to fix the index issues as well as the return value of the boolean functions.

My main problem was that I was not breaking out of the previous loop after moving.

Easily solved by this if statement

if (MoveOnBoard(i) && MoveHasNotBeenMade(i))
        {
            MoveKnight(i);
            break;
        }

I was trying to achieve this by telling the compiler

        if (i == 8)
            movesStillExist = false;

As pointed out by @Aziuth this condition will never be met because a move at that index does not exist.

So instead for my purposes I changed that condition to be

            if (i == 7)
            movesStillExist = false;

Also for the index issues my logic was a little off

if (((0 <= futureRow) && (0 <= futureColumn)) && ((futureRow < 8) && (futureColumn < 8)))
    return true; // if the future row and column are in bounds then return true
return false; // else the default is false

Also, my code is not idealistic for c++.

Having so many global variables and not enough commenting.

Please understand that the use of single and multidimensional arrays are required due to this being a challenge for my c++ course.

bool MoveOnBoard(int& moveChoice)
{
    int futureRow = currentRow + vertical[moveChoice];
    int futureColumn = currentColumn + horizontal[moveChoice];

    if (((0 <= futureRow) && (0 <= futureColumn)) && ((futureRow < 8) && (futureColumn < 8)))
        return true;
    return false;
}

bool MoveHasNotBeenMade(int& moveChoice)
{
    int futureRow = currentRow + vertical[moveChoice];
    int futureColumn = currentColumn + horizontal[moveChoice];

    if (chestBoard[futureRow][futureColumn] == 0)
        return true;
    return false;
}

void PossibleMoves()
{
    bool movesStillExist = true;

    while (numberOfMovesMade < 65 && movesStillExist)
    {
        for (int i = 0; i < 8; i++)
        {
            if (MoveOnBoard(i) && MoveHasNotBeenMade(i))
            {
                MoveKnight(i);
                break;
            }
            if (i == 7)
                movesStillExist = false;
        }
    }
}

void MoveKnight(int &moveChoice)
{
    // Takes in the int moveNumber as a parameter
    // MoveNumber(or case) must be between 0 and 7
    // if there is not a case for the value then the knight will not move
    chestBoard[currentRow][currentColumn] = numberOfMovesMade;
    numberOfMovesMade++;
    currentRow += vertical[moveChoice];
    currentColumn += horizontal[moveChoice];
    chestBoard[currentRow][currentColumn] = numberOfMovesMade;
}

"not enough commenting" is not really an issue. Comments are only necessary when the code doesn't explain itself. Good names can make it self-explanatory. In your case, they are. Although some are misleading - PossibleMoves() sounds like something that in some way returns a list of allowed moves but does not change anything, however what it actually does is to move the knight. The global variable thing is an indicator that you really really want a class. If you want, I can give you a class layout that tries to stay as close as possible to your code. — Aziuth, Feb 16 '17 at 18:11

Knight Tour Index and Moving Algorithm Bugs

2 Answers2