8 Puzzle solver A* search

I recently created this for a coursework assignment. I am always looking to learn and would like some feedback on my code. Is there any OO principles I'm going against or anything irritating I'm doing?

Search Class:

```
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

/**
Represents the A algorithm
*
* @author Paymon Sattarzadeh
* @date 27/11/2014
* @time 16:19
*/
public class Search {

/**
* Active instance of Board
*/
private Board board;
/**
* Active instance of Board representing the goal state
*/
private Board goalBoard;
/**
* g value
*/
private int g = 0;
/**
* Is the search complete
*/
private boolean searchComplete = false;
/**
* f values for the set of nodes currently being considered
*/
ArrayList nodeFValues = new ArrayList();
/**
* Tiles for the set of nodes currently being considered
*/
ArrayList nodeTiles = new ArrayList<>();
/**
* The last move
*/
private int lastMove = 0;

/**
* Construct the Search instance
*
* @param board
* The board
*/
public Search(Board board, Board goalBoard){
this.board = board;
this.goalBoard = goalBoard;

searchAlgo();
}

/**
* Main search algorithm
*/
private void searchAlgo() {

/ If there are no tiles out of place then you are at goal state /
if (tilesOutOfPlaceHeuristic(board) == 0) {
System.out.println("Complete");
board.print();
searchComplete = true;

return;
}

/ Checks if the puzzle is solvable /
if (isSolvable())
nodeManager();
else {
System.out.println("This puzzle is not solvable");

return;
}

ArrayList listt = new ArrayList();

/ Adds all the tiles with equal f values to the listt arraylist /
for (int value : nodeFValues) {

if (value == Collections.min(nodeFValues))
listt.add(nodeTiles.get(nodeFValues.indexOf(value)));

}

/*
* If there are nodes with the same f value

Your indentation is a little inconsistent. I don't know if that's how it looks in your IDE or is an artifact of the copy and paste. In general code is easier to read when formatted consistently. For example, your declaration of the Search class is indented more than its fields. Usually the opposite would be true.

ArrayList nodeFValues = new ArrayList();

You'd normally have the left side defined by the interface rather than the implementation. That way, you can pass it to anything that expects the interface and can easily change your implementation at just a single place. It also helps remind you of what is part of the interface and what is specific to the implementation. You can't accidentally access something specific to the implementation through an interface object.

private List nodeFValues = new ArrayList<>();

Unless you are doing something clever that I missed, you probably want to use the <> operator to indicate that you want this to be a list of the same type as the left side definition.

You also want to declare this as private almost always. You could also declare this as final if you never assign a new list to it. Yes, you will still be able to clear it and add elements to the list if you declare it final. You just won't be able to assign to the variable itself.

public Search(Board board, Board goalBoard){
    this.board = board;
    this.goalBoard = goalBoard;

    searchAlgo();
}

As a general rule, you don't want to do processing in your constructor. You don't show your main function, so I don't know what should change there. It could be as simple as calling searchAlgo right after calling the constructor.

Note: this is a very common thing to try when first encountering objected-oriented principles. It's just that doing this makes it harder to reuse the code in different ways. For example, you might have reasons to do something else before running the search. As a general rule, constructors should just allocate and initialize variables. They should not attempt to read input or process things.

private void searchAlgo() {

So searchAlgo seems short for search algorithm. I tend to be against abbreviations, as they slow code reading more than they speed code writing. After school, you will find that you spend more time reading code (both yours and that of others) than you do writing code. But that's not actually the biggest concern that I have with that name.

As a general rule, classes and objects should get noun names and methods should get verbs. This makes makes the code read more naturally. Search algorithm is an adjective followed by a noun, so I wouldn't use it as a method name. You could name the class SearchAlgorithm, but I think that I might name it Searcher or BoardSearcher. You can then name the method either search (specific to this problem) or the more generic process. Since you presumably don't have a standard to which to adhere, I'd probably go with search. This isn't the optimal use case for something like process.

if (tilesOutOfPlaceHeuristic(board) == 0) {

Again, I would name this as as a verb, e.g. countTilesOutOfPlace. You're actually counting the tiles, so you don't need to call it a heuristic. You might also define this in the Board class, as it is something you use for searching, not part of the search itself.

if ( board.countTilesOutOfPlace() == 0 ) {

That also resolves another issue where you were calling the function with a parameter board even though board is a member of the class.

if (isSolvable())
        nodeManager();
    else {

You do two things here. First, the Java standard prefers that you always use braces (although the language allows the other form. More seriously (at least to me), you are mixing styles in the same control statement. Even in languages where the single statement form is allowable if not preferred, it's better not to use both forms in the same if/else construct.

As Laura noted, isSolvable really should be a method on the Board class, not the search class.

Another thing is that you shouldn't need to do isSolvable on each call to the function. Check if a board is solvable before passing the board to search at all.

ArrayList listt = new ArrayList();

I don't like the name listt. I'm guessing that this is short for list of tiles, which is redundant. We can already see that it's a list of tiles. What's the purpose of this list of tiles? As a default name if nothing better presents itself, I'd prefer just tiles which is simpler. Also, I'd again rewrite this:

List listt = new ArrayList<>();

Favor interface declarations and avoid typeless lists.

```
/ Adds all the tiles with equal f values to the listt arraylist /
for (int value : nodeFValues) {

if (value == Collections.min(nodeFValues))
listt.add(nodeTiles.get(nodeFValue

StackExchange Code Review Q#72047, answer score: 2