HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Moving across my (sturdier?) Bridge

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
bridgesturdiermovingacross

Problem

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:
*
* "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"
*
*/
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 determine

Solution

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 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 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);
}

Code Snippets

public Position swap() {
    return this == Position.EAST ? Position.WEST : Position.EAST;
}
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);
}

Context

StackExchange Code Review Q#117784, answer score: 4

Revisions (0)

No revisions yet.