Refining AI movement logic

Question

I have the below class which moves the AI towards the given plant, this works well however it feels really messy.

Any input as to a better way to lay out the logic would be really grateful, the logic follows the idea that we will take the x and y positions of the AI from the plants position, if the values are positive we add 30 and if it is negative we take away 30

If you need any more explaining of the logic let me know.

private void movePosistion(Plant p) {
 /*
 * set which direction to move,the number generated relates to the
 * direction as below:
 * 1 2 3 
 * 4 5
 * 6 7 8
 */
 int xdiff = p.getXpos() - xpos;
 int ydiff = p.getYpos() - ypos;
 if (xdiff > 0){
 if (ydiff > 0){
 //8
 xpos += 30;
 ypos += 30;
 }else if(ydiff < 0){
 //3
 xpos += 30;
 ypos -= 30;
 }else{
 //5
 xpos += 30;
 }
 }else if(xdiff < 0){
 if (xdiff > 0){
 //6
 xpos -= 30;
 ypos += 30;
 }else if(xdiff < 0){
 //1
 xpos -= 30;
 ypos -= 30;
 }else{
 //4
 xpos -= 30;
 }
 }else{
 if (ydiff < 0){
 //7
 ypos -= 30;
 }else{
 //2
 ypos += 30;
 }
 }
 if (xpos > 720)
 xpos = 1;
 if (ypos > 720)
 ypos = 1;
 if (xpos < 1)
 xpos = 720;
 if (ypos < 1)
 ypos = 720;
 }

• coredump: Is your position always "1 + 30x", for some x? I think this is a decision that could eventually lead to off-by-one errors; generally, if you have a width of 720 points, it is better to have coordinates ranging from 0 to 719; math operations then fit more naturally, without having to add or remove 1; for example, you could use the modulus operator : xpos = xpos % 720 in your last lines.
• coredump: I edited my answer to provide more explanations.
• coredump: I just noticed that the function is called "movePosistion"; I corrected it in my answer. Btw, thanks for accepting it.

Answer 1

Here is my refactoring:

private void movePosition(Plant p) {
 xpos += Integer.signum(p.getXpos() - xpos) * DELTA_X;
 ypos += Integer.signum(p.getYpos() - ypos) * DELTA_Y;
 xpos = Math.floorMod(xpos, MAX_X);
 ypos = Math.floorMod(ypos, MAX_Y);
}

With the right import static this can also be:

private void movePosition(Plant p) {
 xpos = floorMod(xpos + signum(p.getXpos() - xpos) * DELTA_X, MAX_X);
 ypos = floorMod(ypos + signum(p.getYpos() - ypos) * DELTA_Y, MAX_Y);
}

Signum

signum implements the sign function, which gives -1, 0 or 1 for negative integers, zero and positive integers respectively. It encodes the original logic in a very short and readable expression. The sign is multiplied by the appropriate amount of units (constants are not detailed in the code, btw).

I have nothing against decision tables in principle (see rolfl's answer), but I don't think this is necesary here. In his answer, palacsint cited "Code Complete". I can do that too!

From Code Complete 2nd Edition, Chapter 19: General Control Issues, page 431:

Use decision tables to replace complicated conditions

Sometimes you have a complicated test involving several variables. [...]

... and sometimes you don't ;-)

Modulus

The wrap-around behaviour of the original code can be achieved with a modulus operation. Note that you cannot just use the % operator in Java because it computes the remainder, which can be negative when your position goes below zero. The floorMod operation actually computes modular arithmetic.

Now, you might think: this is wrong, the original code does not work like this! Yes, but let me explain:

• First, OP's coordinates range from 1 to 720 (both inclusive). I have an issue with this and I deliberately changed the approach here. The reason is that, in the original code, instead of using a coordinate space that have the origin at (0,0), the origin is translated at (1,1).

  Most of the time, you end up having to add or substract 1 to you operations. That can eventually lead to off-by-one errors. If coordinates are from 0 to 719, however, the wrap-around logic is simply given by the modulus operation.

• "But the original behavior is not like a modulus!", you might say. Why do you say this? because, suppose x is 710, and then I add 30: with modulus, I goes back to 20, whereas using OP's code, I would have 1 because when we are out of bounds, we go back to the minimal (or maximal) position.

  To that, I reply that you never are at position 710, but only at 0, 30, 60, ..., 690. At least, this is what I understand from OP's code, where the object seems to move on a grid, and not freely around. I suppose the object is always located initially at a multiple of 30, and then can only move by 30 units.

  If I am wrong, then (1) sorry, and (2) yes, the modulus is not exactly the good answer; you might better use the boundPos function from rolfl.

• DevWithZachary: I really really like this answer primarily for the fact of its simplicity, all the answers given are great and very different approach to take.

Answer 2

I agree with @rolfl that you need to extract methods and use a decision table.

From Code Complete 2nd Edition, Chapter 19: General Control Issues, page 431:

Use decision tables to replace complicated conditions

Sometimes you have a complicated test involving several variables. It can be helpful to use a decision table to perform the test rather than using ifs or cases. A decision-table lookup is easier to code initially, having only a couple of lines of code and no tricky control structures. This minimization of complexity minimizes the opportunity for mistakes. If your data changes, you can change a decision table without changing the code; you only need to update the contents of the data structure.

I would be more verbose here for the sake of readability. First, an enum for the possible differences:

public enum Difference {
 ZERO,
 BELOW_ZERO,
 ABOVE_ZERO
}

Then a class which contains the available rules:

public class MoveLogic {
 private final Map<Rule, Move> rules = new LinkedHashMap<>();
 public MoveLogic() {
 addRule(Difference.ABOVE_ZERO, Difference.ABOVE_ZERO, 30, 30); // 8
 addRule(Difference.ABOVE_ZERO, Difference.BELOW_ZERO, 30, -30); // 3
 addRule(Difference.ABOVE_ZERO, Difference.ZERO, 30, 0); // 5
 addRule(Difference.BELOW_ZERO, Difference.ABOVE_ZERO, -30, 30); // 6
 addRule(Difference.BELOW_ZERO, Difference.BELOW_ZERO, -30, -30); // 1
 addRule(Difference.BELOW_ZERO, Difference.ZERO, -30, 0); // 4
 addRule(Difference.ZERO, Difference.BELOW_ZERO, 0, -30); // 7
 addRule(Difference.ZERO, Difference.ABOVE_ZERO, 0, 30); // 2
 }
 private void addRule(Difference xDifference, Difference yDifference, int x, int y) {
 rules.put(new Rule(xDifference, yDifference), new Move(x, y));
 }
 public Move getMove(Difference xDifference, Difference yDifference) {
 return rules.get(new Rule(xDifference, yDifference));
 }
}

Usage:

final Difference xDifference = calculateDifference(plant.getXpos(), xpos);
final Difference yDifference = calculateDifference(plant.getXpos(), ypos);
final Move move = moveLogic.getMove(xDifference, yDifference);
xpos += move.getX();
ypos += move.getY();

Finally, the helper method to create Difference references:

private Difference calculateDifference(final int plantPosition, final int position) {
 final int difference = plantPosition - position;
 if (difference > 0) {
 return Difference.ABOVE_ZERO;
 }
 if (difference < 0) {
 return Difference.BELOW_ZERO;
 }
 return Difference.ZERO;
}

as well as Rule:

public class Rule {
 private Difference xDifference;
 private Difference yDifference;
 public Rule(Difference xDifference, Difference yDifference) {
 this.xDifference = xDifference;
 this.yDifference = yDifference;
 }
 // generate hashCode and equals, the Map needs it!
}

and Move:

public class Move {
 private int x;
 private int y;
 public Move(int x, int y) {
 this.x = x;
 this.y = y;
 }
 public int getX() {
 return x;
 }
 public int getY() {
 return y;
 }
}

It might seem too verbose but it avoids repeated conditions as well as magic computations with indexes and arrays.

• coredump: It definitely seems too verbose. I would even say over-engineered, to be honest. Why represent rules at runtime (this is not in the spec)? why encode the concept of Difference in a class? why take a complex approach? Dont add getters/setters that just set and get values: use public fields; you can always evolve towards more complexity if needed, in the future, but only if this becomes necessary. This is just my opinion, hope it helps :-)

Answer 3

Pre-computing the possible moves, and reducing the conditionals to a simple mathematical expression is a nice way to solve problems like this.

Your code also suffers from repeating blocks, and duplicated logic: DRY - Don't Repeat Yourself.

Solve that by doing function extraction.

Finally, your code is full of magic numbers, which are prone to leak bugs.....

Consider the following code, which, though longer than yours in terms of lines, is actually significantly more readable (except for the complicated 2D array, I admit).

private static final int DELTA = 30;
private static final int MINPOS = 0;
private static final int MAXPOS = 720;
private static final int XINDEX = 0;
private static final int YINDEX = 1;
private final int[][] MOVES = {
 {-DELTA, DELTA}, {0, DELTA}, {DELTA,DELTA},
 {-DELTA, 0}, {0, 0}, {DELTA,0},
 {-DELTA, -DELTA},{0, -DELTA},{DELTA,-DELTA},
};
private final int step(final int diff) {
 return diff < 0 ? 0 : (diff > 0 ? 2 : 1);
}
private final int boundPos(int pos) {
 return pos < MINPOS ? MAXPOS : (pos > MAXPOS ? MINPOS : pos);
}
private void movePosistion(Plant p) {
 /*
 * set which direction to move,the number generated relates to the
 * direction as below:
 * 1 2 3 
 * 4 5
 * 6 7 8
 */
 int xdiff = step(p.getXpos() - xpos);
 int ydiff = step(p.getYpos() - ypos);
 // Convert the two x/y directions to an index in to MOVES.
 int index = ydiff * 3 + xdiff;
 xpos += MOVES[index][XINDEX];
 ypos += MOVES[index][YINDEX];
 xpos = boundPos(xpos);
 ypos = boundPos(ypos);
}

• rolfl: @coredump - thanks... and good spotting. You're right. Fixing now.
• coredump: Even though a table is good approach in principle, I don't think this is needed here; see my answer (I reuse boundPos, btw)
• rolfl: @coredump - nice. Your answer too. You should visit the 2nd Monitor chat room to discover another side of Code Review.

Answer 4

Here is my take on it. Since the motions in x and y are independent, I define a class for motion in one direction.

public class Coordinate {
 private final int worldSize;
 private final int stepSize;
 private int value;
 public Coordinate(int worldSize, int stepSize, int initialValue) {
 this.worldSize = worldSize;
 this.stepSize = stepSize;
 if (initialValue < 0 || initialValue >= worldSize)
 throw new IllegalArgumentException();
 this.value = initialValue;
 }
 // TODO getters
 public void moveStepUp() {
 value = (value + stepSize) % worldSize;
 }
 public void moveStepDown() {
 value = (value - stepSize + worldSize) % worldSize;
 }
 public void moveStepCloserTo(Coordinate other) {
 // TODO it would better to consider "closer" through the periodic boundaries too.
 if (this.value < other.value)
 moveStepUp();
 else if (this.value > other.value)
 moveStepDown();
 }
}
public class Coordinate2D {
 private final Coordinate x;
 private final Coordinate y;
 // TODO constructors, getters, moves
 public void moveStepCloserTo(Coordinate2D other) {
 x.moveStepCloserTo(other.x);
 y.moveStepCloserTo(other.y);
 }
}
public class World {
 private Coordinate2D playerPosition;
 private Coordinate2D plantCoordinate;
 // TODO Constructors ...
 // loop over:
 playerPosition.moveStepCloserTo(plantCoordinate);
}

It would change all of your whole code quite a bit. But you can at least use the idea that the motions in x and y are independent.

• coredump: in Coordinate constructor, why not also throw an exception when initialValue is negative?
• toto2: Corrected. I guess many other things would need to be fixed. For example, it's possible at the moment to moveCloser to a coordinate which has a different worldSize.