Bridge, We've Got a Problemo

Now that we can move across the bridge, it's time to finally set up the problem so that it can be solved. A reminder of what the assignment is:


Welcome to the Bridge Crossing Problem. Person Pn can cross the bridge
in n minutes. Only one or two persons can cross at a time because it
is dark, and the flashlight must be taken on every crossing. When two
people cross, they travel at the speed of the slowest person. Devise a
sequence of crossings so that all four people get across the bridge in
no more than 17 minutes.

Here's what I've come up with in a class form. Any suggestions for improvement?

BridgeProblem.java:

```
package bridge;

import java.util.List;
import java.util.ArrayList;

/**
* This class represents the Bridge Crossing problem.
* It provides an introductory message describing the problem,
* stores the problem's possible moves and current state, and
* tests for whether the problem has been successfully solved.
* @author syb0rg
*/
public class BridgeProblem
{
private List moves;
private BridgeState currentState;
/**
* The bridge problem constructor should create the initial bridge state
* object and store it as the problem's current state.
* It should also create the 10 valid bridge move objects and store them
* on an accessible list.
*/
public BridgeProblem()
{
String[] moveNames = new String[]{"P1 crosses alone",
"P2 crosses alone",
"P5 crosses alone",
"P10 crosses alone",
"P1 crosses with P2",
"P1 crosses with P5",
"P1 crosses with P10",
"P2 crosses with P5",
"P2 crosses with P10",
"P5 crosses with P10"

Why are you creating all these classes if you end up hardcoding all the values anyway?

There are two parts especially that I would make more flexible:

String[] moveNames = new String[]{"P1 crosses alone",
                                  "P2 crosses alone",
                                  "P5 crosses alone",
                                  "P10 crosses alone",
                                  "P1 crosses with P2",
                                  "P1 crosses with P5",
                                  "P1 crosses with P10",
                                  "P2 crosses with P5",
                                  "P2 crosses with P10",
                                  "P5 crosses with P10"};

And:

return currentState.getTimeSoFar() <= 17 
        && currentState.getP1Position() == Position.EAST 
        && currentState.getP2Position() == Position.EAST
        && currentState.getP5Position() == Position.EAST
        && currentState.getP10Position() == Position.EAST
        && currentState.getFlashlightPosition() == Position.EAST;

These things indicate, to me, some things that I would change:

• A BridgeMove should not be constructed from a String. It makes more sense to construct with a varargs parameter BridgeMove(String... names), P1 crosses with P2 and such could be the result of calling the toString method (or a getDescription method) of that class.

• Even better instead of using Strings for a person would indeed be to use a class Person with two fields: Name and time.

• A BridgeState should contain a Map where the keys are the name of the person and the value is the position of that person.

• Don't hardcode the parameters of the problem in your constructor, add a factory method that creates an instance with those parameters instead.

Example of factory method: (this can be improved further in various ways)

public static BridgeProblem createStandardProblem() {
     List persons = new ArrayList<>();
     Person p1 = new Person("Adam", 1);
     persons.add(p1);
     Person p2 = new Person("Bert", 1);
     persons.add(p2);
     ...
     List moves = new ArrayList<>();
     moves.add(new BridgeMove(p1));
     moves.add(new BridgeMove(p2));
     moves.add(new BridgeMove(p1, p2));
     ...
     return new BridgeProblem(persons, moves);
}

It's quite easy to make these changes which would make your code much cleaner. Perhaps more importantly: Use a loop in your success method (which perhaps should be named isSuccess btw) to check if all your persons are at the eastmost position.

StackExchange Code Review Q#117981, answer score: 5