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

Organizing code in procedures and functions

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

Problem

I'm writing code that makes the monster depend on value "followHunter", follow or run away from hunter. I have written the following code. Unfortunately it's long and non-optimized. I feel that there is a way to make it simpler by organizing it in procedures and functions but I have no idea how to do it.

In this batch of code I don't like that I have to call moveObject even if the coordinates aren't changed.

public void moveMonster(Map map, Point hunterPosition) {
        Point newCoordinates;
        if (followHunter) newCoordinates = followHunter(map, hunterPosition.x, hunterPosition.y);
        else newCoordinates = runAwayFromHunter(map, hunterPosition.x, hunterPosition.y);
        map.moveObject(x, y, newCoordinates.x, newCoordinates.y, this);
    }


Function checkCollision returns true when monster can move on specified coordinates.
When monster runs away from hunter, direction is random. Then when monster encounters an obstruction it's change direction and tries again. When it encounters an obstruction again then it doesn't move.

```
private Point runAwayFromHunter(Map map, int hunterX, int hunterY) {
int x = this.x;
int y = this.y;
boolean secondAttempt = false;
boolean direction = random.nextBoolean();

if (direction) {
if (x < hunterX) {
if (map.checkCollision(x-1, y)) --x;
else {
direction = !direction;
secondAttempt = true;
}
} else {
if (map.checkCollision(x+1, y)) ++x;
else {
direction = !direction;
secondAttempt = true;
}
}
} else {
if (y < hunterY) {
if (map.checkCollision(x, y-1)) --y;
else {
direction = !direction;
secondAttempt = true;
}
} else {
if (map.checkCollision(x, y+1)) ++y;
else {
direction = !direction;
s

Solution

This could be simplified:

if (x < hunterX) {
        if (map.checkCollision(x-1, y)) --x;
        else {
            direction = !direction;
            secondAttempt = true;
        }
    } else {
        if (map.checkCollision(x+1, y)) ++x;
        else {
            direction = !direction;
            secondAttempt = true;
        }
    }


Basically, depending on the condition x < hunterX,
you either check-and-add 1 or check-and-subtract 1 to x.
This code is equivalent, with less duplication:

int dx = x < hunterX ? -1 : 1;
    if (map.checkCollision(x + dx, y)) x += dx;
    else {
        direction = !direction;
        secondAttempt = true;
    }


You could apply the same for the other checks on y < hunterY.

Next, you could apply the same logic inside if (secondAttempt) { ... }.

Next, you could copy and paste the now simplified code of if (secondAttempt) { ... } into the first set of conditions where you set secondAttempt = true,
and further simplify that too.
As a result, the method body becomes this:

int dx = x < hunterX ? -1 : 1;
    int dy = y < hunterY ? -1 : 1;

    if (direction) {
        if (map.checkCollision(x + dx, y)) x += dx;
        else if (map.checkCollision(x, y + dy)) y += dy;
    } else {
        if (map.checkCollision(x, y + dy)) y += dy;
        else if (map.checkCollision(x + dx, y)) x += dx;
    }

    return new Point(x, y);


This is simpler and easier to understand.

You could apply the same technique in followHunter too.

I recommend to use a different name for local variables than member variables,
to avoid possible confusion and errors.
For example, instead of int x = this.x, how about int newX = this.x.

Code Snippets

if (x < hunterX) {
        if (map.checkCollision(x-1, y)) --x;
        else {
            direction = !direction;
            secondAttempt = true;
        }
    } else {
        if (map.checkCollision(x+1, y)) ++x;
        else {
            direction = !direction;
            secondAttempt = true;
        }
    }
int dx = x < hunterX ? -1 : 1;
    if (map.checkCollision(x + dx, y)) x += dx;
    else {
        direction = !direction;
        secondAttempt = true;
    }
int dx = x < hunterX ? -1 : 1;
    int dy = y < hunterY ? -1 : 1;

    if (direction) {
        if (map.checkCollision(x + dx, y)) x += dx;
        else if (map.checkCollision(x, y + dy)) y += dy;
    } else {
        if (map.checkCollision(x, y + dy)) y += dy;
        else if (map.checkCollision(x + dx, y)) x += dx;
    }

    return new Point(x, y);

Context

StackExchange Code Review Q#71248, answer score: 2

Revisions (0)

No revisions yet.