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

Refining AI movement logic

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

Problem

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){
                //6
                xpos -= 30;
                ypos += 30;
            }else if(xdiff  720)
            xpos = 1;
        if (ypos > 720)
            ypos = 1;
        if (xpos < 1)
            xpos = 720;
        if (ypos < 1)
            ypos = 720;
    }

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#46834, answer score: 16

Revisions (0)

No revisions yet.