Refactoring the Iffy Tractor task There is a test of OO knowledge called Iffy Tractor. The task is to refactor the code and make it as much flexible as possible for the future modifications.

I have placed the code here.

It was taken from here.

This is a kind of a part of the game with a tractor unit, which receives the commands from user. So the possible modifications are:


add different types of Tractors
add more types of movements, available only to selected types of tractors
add some logging for each movement of the tractor


How would you go about refactoring this code? I am not asking for new code of course, just some basic explanation of your idea with a scheme of classes.

Here is my code as a solution too, please criticize it in a most cruel manner possible:

An evident thing is to replace the orientation string with the vectors, thus removing the if-blocks. I will not dwell on this. 

So my interfaces look like this:

public interface Unit {
}
public interface Land {
    void positionUnit(Unit unit, Location location, Orientation orientation);
    Orientation getUnitOrientation(Unit unit);
    Location getUnitLocation(Unit unit);
}
public interface Landscape {
    boolean isAvailable(Location location);
}
public interface GameOperator {
    public void runCommand(String command, Unit unit);
}


How can I implement the command-movement transition inside the GameOperator?


A simple if-block which calculates new position of the tractor based on command string and then changes it inside the Land via setPosition(..).
Implementations of the Action interface. Create a new one for each command or "stamps" the existing one, taken from the pool with actual values, if no multithreading is expected.




public class Move implements Action {
    private Land land;
    private Unit unit;    
    public Move(Unit unit, Land land) {
        this.land = land;
        this.unit = unit;
    }    
    @Override
    public void evaluate() {
        Location oldLocation = land.getUnitLocation(unit);
        Orientation oldOrientation = land.getUnitOrientation(unit);
        Location newLocation = new Location(oldLocation.getX() + oldOrientation.getX(), oldLocation.getY() + oldOrientation.getY());
        land.positionUnit(unit, newLocation, oldOrientation);
    }
}
Move as a classname sounds really weird. It's a verb. Verbs don't make good class names.

Try a noun like MoveAction instead.

Location also seems to be suffering from "Feature Envy" of sorts... you ask the Land about the Unit's location? And you ask the Land about the Unit's orientation too! And then when you need to reposition a Unit, you ask the Land to do this!

Here, take a look at your own move command.

public void evaluate() {
    Location oldLocation = land.getUnitLocation(unit);
    Orientation oldOrientation = land.getUnitOrientation(unit);
    Location newLocation = new Location(oldLocation.getX() + oldOrientation.getX(), oldLocation.getY() + oldOrientation.getY());
    land.positionUnit(unit, newLocation, oldOrientation);
}


You need to get the Location from the Land, then you need to get the Orientation from the Land, and then you need to make the Land reposition a Unit... in order to move it.

I don't have the source for Land or Orientation or Location but I assume the following:

Orientation is an Enum of NORTH, WEST, EAST, SOUTH with two Maps x and y that map an Orientation to a int value of -1, 0 or 1
Land has a 2D int[] array
Location has 2 ints, x and y
Unit is implemented by Tractor


So lets alter this so it makes sense again. Right now, you've simulated what a human does. Look at the board, get the position of the object, look at in what way it's going... Then pick it up, hit each tile it passes, and place it on the last tile it needs to be.

Welcome to the magical world of programming, where board pieces can move on their own.

Unit should contain a move function. I'm not sure how the game works, since there's no instructions anywhere, but I assume it's a game where you give a series of Actions that would do stuff (like moving a unit some place). moveForwards in the original source code reveals this to me.

So a tractor would only move forward.

I suggest a radically different design from what you've suggested.

Assign responsibilities to each class.

Land manages the Locations.

Location is a value class, it contains x, y. These can't be altered.

Unit has a Location and Orientation.

With this new design, if you want to move, you ask the Unit and give it the Land to do so.
Unit then queries the Land in regards to obstacles and such. Then it moves by requesting a new Location FROM Land.

So why is this important?

Because right now, you've defined Move to be a pretty rigid Action. There's only one way to move. And that way ignores all obstacles (I have the feeling it even ignores the Land's size!). If you were to add obstacles later this code would come crashing down as you have to ask the Land for ALL it's units, iterate over their locations (and possibly orientations, if they're moving at the same time), and somehow calculate if it's possible, and then do the move. And you have to do this for each move, because you can't store state in between.