HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Sliding block puzzle, A*

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
blockslidingpuzzle

Problem

I'm a beginner programmer and I'm hoping someone would be willing to take a look at my code and tell me how to improve my coding style and development approach. Sorry about not commenting, I will try to get into the habit of commenting.

board.h

#include 
#include 

using namespace std;

class board{
    int values[5][5], blankX, blankY, parent, index, manhattan;

public:

    board(int _values[5][5], int _blankX, int _blankY, int _index, int _parent);

    void printBoard();
    bool checkUp();
    bool checkDown();
    bool checkLeft();
    bool checkRight();
    void createChild(vector &_children, int &_index, int &_count);
    void exchangeUp();
    void exchangeDown();
    void exchangeLeft();
    void exchangeRight();
    int nextLevel(vector &_children, int &_index, board _goal, int &parentCount);
    void findSolution(vector &_vector, int &_index, board _goal);
    bool compare(board _goal);
    int getParent(){return parent;};
    void moveUp(){blankX--;};
    void moveDown(){blankX++;};
    void moveLeft(){blankY--;};
    void moveRight(){blankY++;};
    int getBlankX(){return blankX;};
    int getBlankY(){return blankY;};
    void setManhattan(board _goal);
    int getManhattan(){return manhattan;};
};


board.cpp

```
#include "board.h"
#include

board::board(int _values[5][5], int _blankX, int _blankY, int _index, int _parent){
for (int i=0; i& _children, int& _index, int & _count){
_children.push_back(board(values, blankX, blankY, index, _index));
_count++;
}

void board::exchangeUp(){
values[blankX][blankY]=values[blankX-1][blankY];
values[blankX-1][blankY]=0;
}

void board::exchangeDown(){
values[blankX][blankY]=values[blankX+1][blankY];
values[blankX+1][blankY]=0;
}

void board::exchangeLeft(){
values[blankX][blankY]=values[blankX][blankY-1];
values[blankX][blankY-1]=0;
}

void board::exchangeRight(){
values[blankX][blankY]=values[blankX][blankY+1];
values[blankX][blankY+1]=0;
}

int board

Solution

Style:

Why to many get/set (ers).

Your methods should be actions that perform actions on your object and mutate state. You should not expose your state (even through getters) as this binds your implementation (tightly couples) your implementation to that interface.
Code:

This:

bool board::checkUp(){
    if (values[blankX-1][blankY]==-1) {
        return false;
    }else {
        return true;
    }
}


Is easier to write (and read) as:

bool board::checkUp() { return values[blankX-1][blankY] != -1;}


And this:

void board::exchangeUp(){
    values[blankX][blankY]=values[blankX-1][blankY];
    values[blankX-1][blankY]=0; 
}


Is easier to read.write as:

void board::exchangeUp()
{
    std::swap(values[blankX][blankY], values[blankX-1][blankY]); 
    --blankX;
}


But why are you allowing an external entity to blindly change your state. You need to validate that the move is good. You should never trust the code using you to blindly always be correct. Validate the move.
Exposing State

void board::createChild(vector& _children, int& _index, int & _count){
    _children.push_back(board(values, blankX, blankY, index, _index));
    _count++;
}


To me the vector should be a private member (or method member) of the board used for solving the problem. You should not be passing this in from the outside. Also the count variable seems redundant. The count will be equivalent to size() of the vector.
Printing

It's ok to have a print method. But you should pass it the stream you want to print too.

void board::printBoard(std::ostream& out) const


I would also add your own custom operator<< to make sure your object can be printed like other objects. It also helps with standard algorithms where you can use the operator<< in a standard way for printing serialization etc.

std::ostream& operator<<(std::ostream& stream, board const& data)
{
     data.printBaord(stream);
     return stream;
}

Code Snippets

bool board::checkUp(){
    if (values[blankX-1][blankY]==-1) {
        return false;
    }else {
        return true;
    }
}
bool board::checkUp() { return values[blankX-1][blankY] != -1;}
void board::exchangeUp(){
    values[blankX][blankY]=values[blankX-1][blankY];
    values[blankX-1][blankY]=0; 
}
void board::exchangeUp()
{
    std::swap(values[blankX][blankY], values[blankX-1][blankY]); 
    --blankX;
}
void board::createChild(vector<board>& _children, int& _index, int & _count){
    _children.push_back(board(values, blankX, blankY, index, _index));
    _count++;
}

Context

StackExchange Code Review Q#12391, answer score: 3

Revisions (0)

No revisions yet.