Minimax implementation not working with recursion and C++

Question

I basically have a game board, and the player can not move to the same spot more than once, or you lose. The player can move by -1, +1, -16, or +16 only. I want to use minimax to solve this problem, and receive the inputs used by minimax so I can see how it was done.

Here is how I am currently doing it

#pragma optimize("", off)

#include <stdio.h>
#include <Windows.h>

#define hitcount 4

#define POINT 1
#define MINE 2
#define END 3

//4 targets: hit all 1's
//player/mine is 2
//game ender is 3
int Board[] = {
    0, 0, 0, 0, 0, 0, 0,
    1, 0, 0, 0, 0, 1, 0,
    2, 0, 0, 1, 0, 0, 1,
    0, 0, 0, 0, 0, 0, 0,
    3, 0, 0, 0, 0, 0, 0
};

static const int choices[] = {
    -1, 1, -16, 16
};

struct mb {
    int end;
    int score;
    int* moveset;
};

mb solve(int Position, int *Board, int BoardSize, int *MoveSet = NULL, int Offset = 0, int Choice = 0, int Score = 0, bool tree = true) {
    if( tree ) { //tree
        int *BoardCopy = new int[BoardSize];        

        for( int i = 0; i < sizeof(choices); i++ ) {
            MoveSet = new int[256];
            for( int i = 0; i < 256; i++ )
                MoveSet[i] = 0xFF;

            memcpy(BoardCopy, Board, BoardSize);
            mb m = solve(Position, BoardCopy, BoardSize, MoveSet, Offset, i, Score, false);

            if( m.moveset != NULL ) {               
                if( m.end == 1 && m.score == hitcount ) {
                    delete[] BoardCopy;
                    return m;
                }               

            }

            delete[] MoveSet; //this is apparently causing problems??
        }

        delete[] BoardCopy;

    }
    else { //branch
        mb m = {0, 0, MoveSet};

        Position += choices[Choice];
        m.moveset[Offset] = choices[Choice];
        if( Position < 0 || Position >= BoardSize || Board[Position] == MINE || (Board[Position] == END && Score != hitcount) ) {
            m.moveset = NULL;
            return m;
        }
        else if( Board[Position] == POINT ) {
            m.score++;
        }
        else if( Board[Position] == END ) {
            m.end = 1;
            return m;
        }

        Board[Position] = MINE; 

        return solve(Position, Board, BoardSize, m.moveset, Offset + 1, Choice + 1, m.score);

    }

    mb m = {0, 0, NULL};
    return m;
}


int main() {

    int Position = 0;
    for( int i = 0; i < sizeof(Board); i++ ) {
        if( Board[i] == MINE ) Position = i;
    }

    mb m = solve(Position, Board, sizeof(Board));
    if( m.end == 1 && m.moveset != NULL && m.score == hitcount ) {
        printf("SUCCESS!\n");
    }

    getchar();
    return 0;
}

However, it doesn't work. I cannot figure out why. The algorithm looks like it should work, and I the problem looks to be memory cleanup related. My VC++ studio breakpoints at _CrtIsValidHeapPtr after calling solve several times

jxh · Answer 1 · 2013-09-28T03:19:22.030

I cannot attest to the correctness of the algorithm. But, there are coding errors that will lead to undefined behavior that you should address.

The primary issue is that you are confusing the size of the array with the number of elements in the array. In your main(), you should use sizeof(Board)/sizeof(*Board) to determine the number of array elements for Board, otherwise you will read outside the array boundary of Board. Then, you pass sizeof(Board) as the BoardSize to solve, but again, this is not the right value to use when you are checking to see if Position is within bounds. Your outer for loop also computes the number of array elements for choices incorrectly.

As a secondary issue, you are overshadowing the i loop variable name with another variable named i in the inner for loop in solve(). Just to avoid confusion, it should probably be renamed.

So, with these in mind, I would change in main():

    for( int i = 0; i < sizeof(Board)/sizeof(*Board); i++ ) {
        if( Board[i] == MINE ) Position = i;
    }

    mb m = solve(Position, Board, sizeof(Board)/sizeof(*Board));

Then, I would change in solve():

        for( int i = 0; i < sizeof(choices)/sizeof(*choices); i++ ) {
            MoveSet = new int[256];
            for( int j = 0; j < 256; j++ )
                MoveSet[j] = 0xFF;

            memcpy(BoardCopy, Board, BoardSize*sizeof(int));

Also, you can avoid the new/delete business in your code fairly easily by using std::vector<int> for BoardCopy and MoveSet. For example:

        std::vector<int> BoardCopy(BoardSize);

        //...
            std::copy(Board, Board+BoardSize, BoardCopy.begin());

To pass in an int * parameter as you have in your code, you would pass in &BoardCopy[0], but you should consider modifying your code to take an std::vector<int> &. When you do this, you can remove all calls to delete BoardCopy.

Minimax implementation not working with recursion and C++

1 Answers1