Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    }
share|improve this question
2  
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 Apr 10 at 21:29
    
I edited my answer to provide more explanations. –  coredump Apr 11 at 8:03
    
I just noticed that the function is called "movePosistion"; I corrected it in my answer. Btw, thanks for accepting it. –  coredump Apr 11 at 13:51

4 Answers 4

up vote 12 down vote accepted

Here is my refactoring (see explanations below):

private void movePosition(Plant p) {
    xpos += Integer.signum(p.getXpos() - xpos) * DELTA_X;
    ypos += Integer.signum(p.getYpos() - ypos) * DELTA_Y;

    xpos %= MAX_X;
    ypos %= MAX_Y;
}

1. 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, yet 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 ;-)

2. Remainder (modulus)

The wrap-around behaviour of the original code can be achieved with modulo arithmetic. Note that you can use the %= operator in Java (I re-discovered this when writing the answer). 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 (when we are out of bounds, we go back to the min (resp. max) boundary).

    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.

share|improve this answer
1  
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. –  Zac Powell Apr 11 at 12:54

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);
}
share|improve this answer
1  
@coredump - thanks... and good spotting. You're right. Fixing now. –  rolfl Apr 10 at 20:42
    
Even though a table is good approach in principle, I don't think this is needed here; see my answer (I reuse boundPos, btw) –  coredump Apr 10 at 20:58
1  
@coredump - nice. Your answer too. You should visit the 2nd Monitor chat room to discover another side of Code Review. –  rolfl Apr 11 at 10:51

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.

share|improve this answer
2  
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 :-) –  coredump Apr 10 at 21:11

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.

share|improve this answer
    
in Coordinate constructor, why not also throw an exception when initialValue is negative? –  coredump Apr 10 at 21:35
    
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. –  toto2 Apr 10 at 22:28

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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