4
\$\begingroup\$

I have thought a lot about this but can't come to a proper solution. Also, it's hard to google things like this.

Note: The code itself is irrelevant, it's about how to do such a thing in general.

I have a class named Creature which has the following method:

private void move(int direction, LyxCamera camera, MapLayers mapLayers, ArrayList<Creature> entities) {
    if (!moving) {
        if (direction != NONE) {
            this.direction = direction;

            int goalGridPosX = gridPosX;
            int goalGridPosY = gridPosY;

            if (direction == UP) {
                goalGridPosY = gridPosY + 1;
            } else if (direction == RIGHT) {
                goalGridPosX = gridPosX + 1;
            } else if (direction == DOWN) {
                goalGridPosY = gridPosY - 1;
            } else if (direction == LEFT) {
                goalGridPosX = gridPosX - 1;
            }

            if (goalIsFree(goalGridPosX, goalGridPosY, mapLayers, entities)) {
                stop();
                moving = true;

                /* FROM HERE

                if (direction == UP) {
                    if (goalGridPosY > camera.getGridPosY() + 4) {
                        camera.move(direction, speed);
                    }
                } else if (direction == RIGHT) {
                    if (goalGridPosX > camera.getGridPosX() + 8) {
                        camera.move(direction, speed);
                    }
                } else if (direction == DOWN) {
                    if (goalGridPosY < camera.getGridPosY() + 4) {
                        camera.move(direction, speed);
                    }
                } else if (direction == LEFT) {
                    if (goalGridPosX < camera.getGridPosX() + 7) {
                        camera.move(direction, speed);
                    }
                }

                TO HERE*/

                gridPosX = goalGridPosX;
                gridPosY = goalGridPosY;

                step();
            }
        }
    }

I marked the code I want to extract to a subclass with "FROM HERE TO HERE".

I could place the code in a method and call the empty method, then overwrite the method in the subclass. But that really doesn't seem to be a proper solution. I could also copy the whole method and just remove the marked area in the superclass. This also doesn't seem to be best practice.

I'm sure you guys know exactly what I'm looking for. Oh, and if you have general suggestions to improve the code, feel free to let me know!

Maybe I'm just too tired already, and a solution will come to me in my dreams. Anyways, I'm very curious to see your suggestions!

Many thanks!

Regards Lyxodius

EDIT:

First, I'm very thankful that you guys took the time to give me answers so detailed! My solution was inspired by all of your answers from here and stackoverflow!

Superclass "Creature.java":

private void move(int direction, LyxCamera camera, MapLayers mapLayers,
        ArrayList<Creature> entities) {
    if (!moving && direction != NONE) {
        this.direction = direction;

        int goalGridPosX = gridPosX;
        int goalGridPosY = gridPosY;

        switch (direction) {
        case UP:
            goalGridPosY++;
            break;
        case RIGHT:
            goalGridPosX++;
            break;
        case DOWN:
            goalGridPosY--;
            break;
        case LEFT:
            goalGridPosX--;
            break;
        }

        if (goalIsFree(goalGridPosX, goalGridPosY, mapLayers, entities)) {
            prepareMoving(goalGridPosX, goalGridPosY);
        }
    }
}

protected void prepareMoving(int goalGridPosX, int goalGridPosY) {
    startMoving(goalGridPosX, goalGridPosY);
}

private void startMoving(int goalGridPosX, int goalGridPosY) {
    stop();

    moving = true;

    gridPosX = goalGridPosX;
    gridPosY = goalGridPosY;

    step();
}

Subclass "Player.java":

@Override
protected void prepareMoving(int goalGridPosX, int goalGridPosY) {
    super.prepareMoving(goalGridPosX, goalGridPosY);
    moveCamera();
}

private void moveCamera() {
    if (direction == UP
            && gridPosY > camera.getGridPosY() + MINIMUM_VERTICAL_OFFSET) {
        camera.move(direction, speed);
    } else if (direction == RIGHT
            && gridPosX > camera.getGridPosX()
                    + MINIMUM_HORIZONTAL_OFFSET_RIGHT) {
        camera.move(direction, speed);
    } else if (direction == DOWN
            && gridPosY < camera.getGridPosY() + MINIMUM_VERTICAL_OFFSET) {
        camera.move(direction, speed);
    } else if (direction == LEFT
            && gridPosX < camera.getGridPosX()
                    + MINIMUM_HORIZONTAL_OFFSET_LEFT) {
        camera.move(direction, speed);
    }
}
\$\endgroup\$
0

2 Answers 2

4
\$\begingroup\$

Reduce if statement nesting

You have some if statements which can be rewritten a lot better using &&/|| and early return:

if (moving || direction == NONE) {
    return; // already moving or no direction given
}

With this, we already got rid of two levels of nesting.

You can also combine the camera movement ifs:

        if (direction == UP && goalGridPosY > camera.getGridPosY() + 4) {
            camera.move(direction, speed);                    
        }
        // [...etc...]

Misc

  • goalGridPosX = gridPosX + 1; can be written shorter as goalGridPosX++; and goalGridPosY = gridPosY - 1; could be goalGridPosY--; (it's also less confusing. you already assigned gridPosX to goalGridPosX)
  • don't hardcode magic numbers, store the values in fields instead. Right now, it's very unclear why you have 4, 8, and 7 in your code.

Move code to subclass

My first question would be: why do you want to do that? So that you can override it, because different objects behave differently with the camera?

And is there anything that speaks against the simple approach:

// replace code with:
moveCamera(direction, speed, goalGridPosX, goalGridPosY);

// add abstract method:
public abstract void moveCamera(int direction, int speed, int goalGridPosX, int goalGridPosY);

// in subclass:
@Override
public void moveCamera(int direction, int speed, int goalGridPosX, int goalGridPosY) {
    // the code
}
\$\endgroup\$
0
3
\$\begingroup\$

The design pattern you are looking for is called a Template method pattern and this is what you were trying to do. The definition implies the creation of an abstract or template method and overriding inside sub-classes to specialize behavior.

Consider the following example for making a house:

public abstract class HouseTemplate {

    //template method, final so subclasses can't override
    public final void buildHouse(){
        buildFoundation();
        buildWalls();
        buildWindows();
        System.out.println("House is built.");
    }

    //default implementation
    private void buildWindows() {
        System.out.println("Building Glass Windows");
    }

    //method to be implemented by subclasses
    public abstract void buildWalls();

    private void buildFoundation() {
        System.out.println("Building foundation with cement,iron rods and sand");
    }
}

And the following classes for a WoodenHouse and a GlassHouse

public class WoodenHouse extends HouseTemplate {

    @Override
    public void buildWalls() {
        System.out.println("Building Wooden Walls");
    }

}

public class GlassHouse extends HouseTemplate {

    @Override
    public void buildWalls() {
        System.out.println("Building Glass Walls");
    }

}

When the WoodenHouse object is built the output is:

"Building foundation with cement,iron rods and sand"
"Building Wooden Walls"
"Building Glass Windows"
"House is built."

While when the GlassHouse is built the output is:

"Building foundation with cement,iron rods and sand"
"Building Glass Walls"
"Building Glass Windows"
"House is built."

Example taken from here. This shows that when a sub-class is used its method is used in place of the one in the super-class.

Therefore move that piece of code into its own method moveCamera() and override in any class you want the code to be specialized for.

public void moveCamera(int direction, int speed, int goalGridPosX, int goalGridPosY) {

}

You should make this method abstract if the class itself is abstract, this will force you to override it in the sub-classes.

Directions should be stored in an enum like so:

enum Direction {
    NONE, UP, DOWN, LEFT, RIGHT
}

You can use a switch statement for checking the direction and change

goalGridPosY = gridPosY + 1;
goalGridPosY = gridPosY - 1;

Into its equivalent

goalGridPosY = ++gridPosY
goalGridPosY = --gridPosY;

Therefor your code for moving will look like this:

switch (direction) {
    case Direction.UP:
        goalGridPosY = ++gridPosY;
        break;
    case Direction.RIGHT:
        goalGridPosX = ++gridPosX;
        break;
    case Direction.DOWN:
        goalGridPosY = --gridPosY;
        break;
    case Direction.LEFT):
        goalGridPosX = --gridPosX;
        break;
}

You can also combine the first two if statements at the start of the method:

if (!moving && direction != Direction.NONE) {
    // Do code stuffs here
}

You can return early however this wouldn't give you the flexibility of doing something if you are moving or direction == Direction.NONE.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your very detailed answer! I edited the OP to add my solution, it was inspired by your template suggestion! I also use a switch now! \$\endgroup\$
    – Lyxodius
    Commented Oct 18, 2014 at 16:50

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.