Solve the game "Flux"

The Program

I wrote this program as an answer to a question I thought of for Programming Puzzles and Code Golf. To avoid cross-posting, I will only summarize the requirements:

• Solve a game in a 3x4 grid that is similar to the fifteen puzzle, except there are colors instead of numbers, and some colors are duplicated; 4 red, 4 yellow, 2 blue, 1 gray, 1 empty

• The goal is to make the top row exactly match the bottom row.

• Input is in the other thread; basically, input the 12 cells (a permutation of RRRRYYYYBBG_ in one line

• Output the moves (a string of LRUD), and then the resulting board.

Why Code Review?

Once I had it working, I decided this would be a really good sort of program for me to get some review on. I cleaned it up and turned it into something that I would be willing to turn into production (there would be some extra error handling, but that part isn't that hard to add in and is boring). There is nothing in particular that I'm looking for; I've worked really hard to polish my programming skills from books like Go4, Refactoring, and Clean Code, and I want to make sure I understand most of those concepts and am applying them correctly. Note: This program is too short for me to bother with a DI framework, but normally I do all my programming beyond a certain complexity level with Guice. Also; this code depends on Guava, but not for all that much, just Preconditions and the java.util factory methods.

FluxSolver.java (main class)

```
package org.durron597.codereview;

import static com.google.common.base.Preconditions.checkArgument;

import java.util.*;

import com.google.common.collect.Maps;
import com.google.common.collect.Sets;

public class FluxSolver implements Runnable {
public static final String DEFAULT_TEMPLATE = "RRRRYYYYBBG_";

private final Map allBoards = Maps.newHashMap();
private final String template;

private Set scoredBoards = Sets.newHashSet();

public FluxSolver() {
this(DEFAULT_TEMPLATE);

FluxSolver

I don't see why this class needs to be a Runnable. It could be just a simple class with a solve method, no?

You're not using the command line arguments. It would be nice to offer a mode in which you can give the board string on the command line. Without arguments, the game could work in interactive mode like it is now, otherwise it could be batch mode.

You're not validating the input lines. If I make a mistake and enter an incorrect board string the game crashes with an NPE.

It would make sense to call getBoardFromRawString when you initialize currentBoard in printSolution. And the method could perform input validation.

It could be interesting to let the command line control the game dimensions as well. So for example you could run it with parameters 3 4 RRRRYYYYBBG_ to behave like the current game.

I don't have too much time now to look closely, but what is the number 20 in solveAllBoards?

Board

In isSolved there is a remnant of hard-coded grid information:

public boolean isSolved() {
    return Arrays.equals(boardCells[0], boardCells[2]);
}

You obviously meant Y_SIZE - 1 instead of "2" there. How about this instead:

public boolean isSolved() {
    return Arrays.equals(boardCells[0], boardCells[boardCells.length - 1]);
}

In the constructor, I found the raw variable name a bit confusing. rawPosition would be more clear.

It might make sense to let the dimensions be constructor parameters instead of static constants.

I think the class is doing a bit too much. It describes a board, and it keeps history and score too. It might be good to create a BoardWithHistory class or something to separate these other responsibilities.

Direction

The values would be slightly more readable if you aligned the moveToEmpty(...) calls the same way for all of them, for example:

LEFT("L") {
    public Board moveSquare(FluxSolver solver, Board input) {
        return input.getEmptyPosition().x == 0 ? null
                : moveToEmpty(solver, input, -1);
    }
},

instead of:

LEFT("L") {
    public Board moveSquare(FluxSolver solver, Board input) {
        return input.getEmptyPosition().x == 0 ? null : moveToEmpty(solver, input,
                -1);
    }
},

The line break in between the method parameters was awkward anyway, with -1 hanging there all alone.

It's a bit strange that moveToEmpty needs a FluxSolver to operate, in a logical sense. To me it's already a bit fishy that a Direction class is aware of Board, but that maybe be forgivable. Depending on a FluxSolver doesn't seem cool. Perhaps you can move the getBoardFromRawString method from FluxSolver to Board instead.

Also in the moveToEmpty method, i is hardly a good name for a Board.

But the biggest problem with this method is that it moves the empty cell in a pretty awkward way. Converting the board's raw string representation to char array, swapping the symbol, then converting back to raw string is not very cool.

Maybe you can refactor this part to work with Board.CellType[][] of the Board in a more direct way, without converting strings and char arrays back and forth. That might bring another benefit too: getting rid of Board.rawstring and Position.raw which look like noise in the class design.

StackExchange Code Review Q#50962, answer score: 4