2
\$\begingroup\$

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:

  1. add different types of Tractors

  2. add more types of movements, available only to selected types of tractors

  3. 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?

  1. A simple if-block which calculates new position of the tractor based on command string and then changes it inside the Land via setPosition(..).

  2. 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);
    }
}
\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ 1) modifying my way: Land may have only set(x,y,orientation) or getters; another option is code land.moveForward(unit). All the ways treat the obstacles and land size right: land throws Exception if the position is unavailable, or may return some success status instead. 2) going your way: how the unit will get the land object? Who will hold the unit coordinates - land or unit? \$\endgroup\$
    – MiamiBeach
    Sep 12, 2014 at 11:47
  • \$\begingroup\$ We've gone off on a design tangent which is not conductive to Code Review... There are two ways for modeling tiles. One is to have the tiles be a linked set of other tiles, indexed by direction. This is a very extensible pattern, considering it supports abstract patterns. You can also model the tiles as a 2D array, which is what you have done. In that case, the wrapper class of the 2D array is responsible for telling you what you need. \$\endgroup\$
    – Pimgd
    Sep 12, 2014 at 11:56
  • \$\begingroup\$ @Dymytry Land has Locations. Unit has a Location. Location may have a Unit. That's my solution to the problem here. \$\endgroup\$
    – Pimgd
    Sep 12, 2014 at 11:58
  • \$\begingroup\$ Pimgd, I just cannot get how the unit will check wether it can step on that part of the land in your solution. unit.moveForward(land) - like this? Or it should have land object injected? How it will consult the land object if there are no obstacles. \$\endgroup\$
    – MiamiBeach
    Sep 12, 2014 at 12:01
  • \$\begingroup\$ @Dymytry Ah crap, I had code before but it ruined my explanation so I removed it. Yes, you should pass Land to Unit so it can move. And unit.moveForward is wrong, unless you have a MoveForwardAction. The rationale for passing Land to Unit: How would you navigate the world without knowing what the world looks like? \$\endgroup\$
    – Pimgd
    Sep 12, 2014 at 12:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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