I think my Bridge's foundation is too fragile

Question

I've just started a new class learning Java, and since it's been a long time since I've programmed in it I thought I'd put up my first project to get back in the swing of things. Here is my first assignment:

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.

So I built a system of classes to handle all of this, these two here are the base two. How can I improve this code? Any new Java 8 features I'm not taking advantage of?

Position.java:

package bridge;

/**
 * An enumerated type whose values indicate sides of the bridge in the bridge problem.
 * Refer to these values using "<b>Position.EAST</b>" and "<b>Position.WEST</b>"
 */
public enum Position { EAST, WEST }

BridgeState.java:

package bridge;

/**
 * This class represents states of the Bridge Crossing problem.
 * It creates new bridge states, tests states for equality,
 * and produces string representations of them.
 * @author your name here
 */
public class BridgeState {

    private final Position p1Location;
    private final Position p2Location;
    private final Position p5Location;
    private final Position p10Location;
    private final Position flashLocation;
    private final int time;
    /**
     * Creates a new bridge state.  
     * Besides storing the positions of the persons and flashlight, a
     * bridge state should also store the time taken so far to cross in
     * integer minutes.
     * @param p1Position position of the person who can cross in 1 minute
     * @param p2Position position of the person who can cross in 2 minutes
     * @param flashlightPosition position of the flashlight
     * @param p5Position position of the person who can cross in 5 minutes
     * @param p10Position  position of the person who can cross in 10 minutes
     * @param timeSoFar time taken so far
     */
    public BridgeState(Position p1Position, 
                       Position p2Position, 
                       Position flashlightPosition, 
                       Position p5Position, 
                       Position p10Position,
                       int timeSoFar) 
    {
        p1Location = p1Position;
        p2Location = p2Position;
        p5Location = p5Position;
        p10Location = p10Position;
        flashLocation = flashlightPosition;
        time = timeSoFar;

    }

    /**
     * Tests this bridge state with another for equality.
     * Two bridge states are equal if the positions of the persons and 
     * flashlight in one state are matched by their positions in the other.
     * Note that the time taken to cross so far is not taken into account
     * when considering equality.
     * @param other the other bridge state to be tested against this one.
     * @return true if this state is equal to the other state, false otherwise
     */
    public boolean equals(BridgeState other) 
    {
        if (p1Location == other.p1Location 
         && p2Location == other.p2Location
         && p5Location == other.p5Location
         && p10Location == other.p10Location
         && flashLocation == other.flashLocation
         ) return true;
        else return false;
    }

    /**
     * Creates a string representation of this state for display to the user
     * trying to solve the problem.
     * Note that the time so far to cross is part of the string representation.
     * @return the string representation of this state
     */
    public String toString() {
        String str = new String();
        str += String.format("%3s |   |%s\n", (p1Location == Position.WEST ? "P1" : ""), (p1Location == Position.EAST ? " P1" : ""));
        str += String.format("%3s |   |%s\n", (p2Location == Position.WEST ? "P2" : ""), (p2Location == Position.EAST ? " P2" : ""));
        str += String.format("%3s |===|%s\n", (flashLocation == Position.WEST ? "f" : ""), (flashLocation == Position.EAST ? " f" : ""));
        str += String.format("%3s |   |%s\n", (p5Location == Position.WEST ? "P5" : ""), (p5Location == Position.EAST ? " P5" : ""));
        str += String.format("%3s |   |%s\n", (p10Location == Position.WEST ? "P10" : ""), (p10Location == Position.EAST ? " P10" : ""));
        str += String.format("Time elapsed so far: %d minutes.\n", time);
        return str;
    }

    public Position getFlashlightPosition() 
    {
        return flashLocation;
    }

    public Position getP10Position() 
    {
        return p10Location;
    }

    public Position getP1Position() 
    {
        return p1Location;
    }

    public Position getP2Position() 
    {
        return p2Location;
    }

    public Position getP5Position() 
    {
        return p5Location;
    }

    public int getTimeSoFar() 
    {
        return time;
    }

}

Answer 1

Let's start with a few basic findings. I know you're mainly aiming at Java 8 features, but maybe I can follow up with more later on when I have more time. I think it might also be interesting to see how this BridgeState class is used later.

Code Style

• Your bracket positions are inconsistent. In some places you are putting them on the same line (public String toString() {), in some places you are putting them on the next line (public Position getFlashlightPosition()).

• Your field is e.g. named p1Location, but your getter is named getP1Position (same goes for the others). Usually getters are named after their corresponding fields (i.e. getP1Location).

• At first glance I'm not a fan of your p1Location, p2Location, etc variables. It might be more beautiful to use something like

  public class Person {
      int timeNeeded;
      Position position;
  }
  

  although I haven't thought about this in detail yet. On the one hand it makes your class more extensible, on the other hand the problem is quite well defined and unlikely to have a dynamic number of people.

• Why is the flashlightPosition interrupting the person locations (read as: why not p1Position, p2Position, p5Position, p10Position, flashlighPosition)?

• String str = new String(); is creating a basically unused object. Just initialize str with your first content.

• You should tag your toString method with @Override.

• Your javadoc states that BridgeState "creates new bridge states", which I cannot see. I would assume more logic behind state creation than a mere constructor (although one can probably argue about that).

• Your Position enum is probably not useful outside a BridgeState context, so you might as well make it an inner class.

Correctness

• When overriding equals (even if slightly altered from the one in Object) also override hashCode. Always. Consider the following example (made up, but it's one of the simplest ways to see the issue and it's not completely unrealistic):

  import java.util.HashMap;
  import java.util.Map;
  
  public class BridgeStateTest {
      public static void main(String[] args) {
          Map<BridgeState, Object> map = new HashMap<>();
          BridgeState bridgeState1 = new BridgeState(Position.EAST, Position.EAST,
                  Position.EAST, Position.EAST, Position.EAST, 0);
          BridgeState bridgeState2 = new BridgeState(Position.EAST, Position.EAST,
                  Position.EAST, Position.EAST, Position.EAST, 0);
          map.put(bridgeState1, "foo");
          System.out.println(bridgeState1.equals(bridgeState2)); // true
          System.out.println(map.containsKey(bridgeState2)); // false
      }
  }
  

  For more information see e.g. this question on Stack Overflow.

• Your equals method throws a NullPointerException for bridgeState.equals(null). While you are not strictly overriding Object.equals, you should try to follow the principle of least astonishment (or rename your method). I'd suggest to change the method parameter to indeed override the method from Object.

Other things: I like the immutability.