8
\$\begingroup\$

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<BridgeMove> 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"};
        currentState = new BridgeState(Position.WEST,
                                       Position.WEST,
                                       Position.WEST,
                                       Position.WEST,
                                       Position.WEST,
                                       0);
        moves = new ArrayList<BridgeMove>();
        for (int i = 0; i < moveNames.length; ++i)
        {
            moves.add(new BridgeMove(moveNames[i]));
        }
    }

    /**
     * Getter (accessor) for this problem's introduction string.
     * @return the introduction string
     */
    public String getIntroduction() 
    {
        return "Welcome to the Bridge Crossing Problem.\n\n" +
               "Person Pn can cross the bridge in n minutes.\n" +
               "Only one or two persons can cross at a time because it is dark,\n" +
               "and the flashlight must be taken on every crossing.\n" +
               "When two people cross, they travel at the speed of the slowest person.\n" +
               "Devise a sequence of crossings so that all four people get across\n" +
               "the bridge in no more than 17 minutes.\n\n";
    }

    /**
     * Getter (accessor) for this problem's list of valid bridge move objects.
     * @return the list of bridge moves
     */
    public List<BridgeMove> getMoves() 
    {
        return moves;
    }

    /**
     * Tests for whether the current state of this problem indicates that the
     * problem has been successfully solved.
     * @return true if the problem has been solved, false otherwise
     */
    public boolean success() 
    {
        return currentState.getTimeSoFar() <= 17 
                && currentState.getP1Position() == Position.EAST 
                && currentState.getP2Position() == Position.EAST
                && currentState.getP5Position() == Position.EAST
                && currentState.getP10Position() == Position.EAST
                && currentState.getFlashlightPosition() == Position.EAST;
    }

    /**
     * Getter (accessor) for this problem's current bridge state.
     * @return the current state
     */
    public BridgeState getCurrentState() 
    {
        return currentState; 
    }

    /**
     * Setter (mutator) for this problem's current bridge state.
     * @param currentState the state to be made this problem's current state
     */
    public void setCurrentState(BridgeState currentState) 
    {
        this.currentState = currentState;
    }

}
\$\endgroup\$
5
\$\begingroup\$

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<String, Position> 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<Person> persons = new ArrayList<>();
     Person p1 = new Person("Adam", 1);
     persons.add(p1);
     Person p2 = new Person("Bert", 1);
     persons.add(p2);
     ...
     List<BridgeMove> 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.

| improve this answer | |
\$\endgroup\$
4
\$\begingroup\$

Your code looks good. There are a couple of comments that I could make:

  • You have hardcoded every possible moves. This probably goes a lot deeper inside your whole code so it might not be easy to change. The problem with this approach is that if tomorrow there are 5 and not 4 people to consider, you have to rewrite everything by adding more checks. As such, this is not very flexible.
  • You have a getter and a setter for the BridgeState. Is the setter really necessary? It is smelly. BridgeProblem is the main class of your problem. It should hide all its nitty details inside, like BridgeState. For example, if you call the getter and then invoke some method foo on BridgeState, consider adding instead the method foo inside BridgeProblem itself, that will delegate to the BridgeState instance. This way, you don't need a setter and it properly encapsulates the role of the BridgeProblem class.
  • In the same idea, I would return an unmodifiable list for the getter of the moves. It ensures that no other code will modify the list and, again, ensures that the state of BridgeProblem is controlled by the class itself.

    public List<BridgeMove> getMoves() 
    {
        return Collections.unmodifiableList(moves);
    }
    
  • Consider renaming the success() to isSuccess(): boolean returning methods generally starts with is (or has sometimes).

Otherwise, it looks clean: your Javadoc is really great and it's good that you're making an effort in this.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.