7
\$\begingroup\$

Building on top of the classes in this question of mine, I've made this class to support the moving of the entities for my given 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.

The thing that is jumping out at me right now of things to be refined is the doMove() method. It looks quite bulky, and somewhat repetitive but I'm not sure about the best way to break it down. Other than that, is there other things that could be improved about my code?


BridgeMove.java:

package bridge;

/**
 * This class represents moves in the Bridge Crossing problem.
 * A move object stores its move name and knows how to apply itself
 * to a bridge state to create a new state representing the result
 * of the move.
 * @author syb0rg
 */
public class BridgeMove 
{
    private final String move;
    /**
     * Constructs a new bridge move object.
     * @param moveName the name of this move.
     * It is an error if the name is not one of the following:
     * <ul>
     * <li> "P1 crosses alone" </li>
     * <li> "P2 crosses alone" </li>
     * <li> "P5 crosses alone" </li>
     * <li> "P10 crosses alone" </li>
     * <li> "P1 crosses with P2" </li>
     * <li> "P1 crosses with P5" </li>
     * <li> "P1 crosses with P10" </li>
     * <li> "P2 crosses with P5" </li>
     * <li> "P2 crosses with P10" </li>
     * <li> "P5 crosses with P10" </li>
     * </ul>
     */
    public BridgeMove(String moveName) 
    {
        move = moveName;
    }

    /**
     * Getter (accessor) for this move object's move name.
     * @return this move object's move name
     */
    public String getMoveName() 
    {
        return move;
    }

    /**
     * Attempts to perform this move on a given bridge state.
     * The move to perform is determined by this object's move name.
     * If the move can be performed a new bridge state object is returned that
     * reflects this move.
     * A move cannot be performed if the flashlight is not on the same side
     * as the crossing person(s), or if a pair of persons who are crossing are not
     * on the same side.
     * If the move cannot be performed <b>null</b> is returned.
     * @param state the bridge state on which this move is to be performed
     * @return a new bridge state reflecting the move, or <b>null</b> if it
     * cannot be performed
     */
    public BridgeState doMove(BridgeState state) 
    {
        if("P1 crosses alone".equals(move) && state.getFlashlightPosition() == state.getP1Position())
        {
            return new BridgeState((state.getP1Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getP2Position(),
                                   (state.getFlashlightPosition() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getP5Position(),
                                   state.getP10Position(),
                                   state.getTimeSoFar() + 1);
        }
        else if ("P2 crosses alone".equals(move) && state.getFlashlightPosition() == state.getP2Position())
        {
            return new BridgeState(state.getP1Position(),
                                   (state.getP2Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   (state.getFlashlightPosition() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getP5Position(),
                                   state.getP10Position(),
                                   state.getTimeSoFar() + 2);
        }
        else if ("P5 crosses alone".equals(move) && state.getFlashlightPosition() == state.getP5Position())
        {
            return new BridgeState(state.getP1Position(),
                                   state.getP2Position(),
                                   (state.getFlashlightPosition() == Position.EAST ? Position.WEST : Position.EAST),
                                   (state.getP5Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getP10Position(),
                                   state.getTimeSoFar() + 5);
        }
        else if ("P10 crosses alone".equals(move) && state.getFlashlightPosition() == state.getP10Position())
        {
            return new BridgeState(state.getP1Position(),
                                   state.getP2Position(),
                                   (state.getFlashlightPosition() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getP5Position(),
                                   (state.getP10Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getTimeSoFar() + 10);
        }
        else if ("P1 crosses with P2".equals(move) && state.getFlashlightPosition() == state.getP1Position() 
                                              && state.getFlashlightPosition() == state.getP2Position())
        {
            return new BridgeState((state.getP1Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   (state.getP2Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   (state.getFlashlightPosition() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getP5Position(),
                                   state.getP10Position(),
                                   state.getTimeSoFar() + 2);
        }
        else if ("P1 crosses with P5".equals(move) && state.getFlashlightPosition() == state.getP1Position() 
                                              && state.getFlashlightPosition() == state.getP5Position())
        {
            return new BridgeState((state.getP1Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getP2Position(),
                                   (state.getFlashlightPosition() == Position.EAST ? Position.WEST : Position.EAST),
                                   (state.getP5Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getP10Position(),
                                   state.getTimeSoFar() + 5);
        }
        else if ("P1 crosses with P10".equals(move) && state.getFlashlightPosition() == state.getP1Position() 
                                               && state.getFlashlightPosition() == state.getP10Position())
        {
            return new BridgeState((state.getP1Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getP2Position(),
                                   (state.getFlashlightPosition() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getP5Position(),
                                   (state.getP10Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getTimeSoFar() + 10);
        }
        else if ("P2 crosses with P5".equals(move) && state.getFlashlightPosition() == state.getP2Position() 
                                              && state.getFlashlightPosition() == state.getP5Position())
        {
            return new BridgeState(state.getP1Position(),
                                   (state.getP2Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   (state.getFlashlightPosition() == Position.EAST ? Position.WEST : Position.EAST),
                                   (state.getP5Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getP10Position(),
                                   state.getTimeSoFar() + 5);
        }
        else if ("P2 crosses with P10".equals(move) && state.getFlashlightPosition() == state.getP2Position() 
                                              && state.getFlashlightPosition() == state.getP10Position())
        {
            return new BridgeState(state.getP1Position(),
                                   (state.getP2Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   (state.getFlashlightPosition() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getP5Position(),
                                   (state.getP10Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getTimeSoFar() + 10);
        }
        else if ("P5 crosses with P10".equals(move) && state.getFlashlightPosition() == state.getP5Position() 
                                               && state.getFlashlightPosition() == state.getP10Position())
        {
            return new BridgeState(state.getP1Position(),
                                   state.getP2Position(),
                                   (state.getFlashlightPosition() == Position.EAST ? Position.WEST : Position.EAST),
                                   (state.getP5Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   (state.getP10Position() == Position.EAST ? Position.WEST : Position.EAST),
                                   state.getTimeSoFar() + 10);
        }
        else return null;
    }
}
\$\endgroup\$
3
\$\begingroup\$

First, notice that you are doing

state.getP1Position() == Position.EAST ? Position.WEST : Position.EAST

a lot in your code. This could be extracted into a method

private static Position switchPosition(Position p) {
    return p == Position.EAST ? Position.WEST : Position.EAST;
}

that you would call with switchPosition(state.getP1Position()).


Your documentation is stating that

It is an error if the name is not one of the following [...]

but you are not checking that this is indeed the case. You could add a validation logic that checks in the given move is one of the correct options.

With regard to your documentation also, the @param doc should be place below the whole explanation so that it gets generated properly:

/**
 * Constructs a new bridge move object.
 * [...]
 * @param moveName the name of this move.
 */
public BridgeMove(String moveName) 
{
    move = moveName;
}

All the if else could potentially be refactored using a dictionary: that is a Map that will hold for every move, a structure that will tell how each BridgeState should be operated upon to return the next one. In this case, however, I don't think it will make the code much clearer. The if conditions read easily and this is very localized (such code won't be present in another part of the code base).

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

You have made the BridgeMove class more complicated than it needs to be.

Currently you are doing the following:

  • Input a String to the class
  • Extract which player should move
  • Add the time it takes the player to move
  • Swap the position for the player, and the flashlight

This has led to a whole lot of code duplication which is harder to maintain. You can't change one of your strings without also changing at least two other things.

What is your data, really? Is your data strings? No, your data consists of: Which persons will move, and how long will it take.

As such, a BridgeMove could contain a List<Person> where a Person contains a String name and int time.


Additionally, expanding on Tunaki's recommendation, you could make your method to swap the position a public non-static method in the Position class:

public Position swap() {
    return this == Position.EAST ? Position.WEST : Position.EAST;
}

Then you could call state.getP1Position().swap().

Or as I would have done it, something like this:

public BridgeState doMove(BridgeState state) {
    Map<Person, Position> positions = state.getMap();
    for (Person person : this.movingPersons) {
        positions.put(person, positions.get(person).swap());
    }
    int maxTime = this.movingPersons.stream().mapToInt(Person::getTime).max().orElse(0);
    return new BridgeState(positions, state.getTimeSoFar() + maxTime);
}
| 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.