A question of design and object responsibility

Question

I have a design and object structuring related question. Here is the problem statement:

1. I have a Robot object which is suppose to traverse the ground on its own. It would be provided movement instructions and it must parse accordingly. For example sample input would be: a. RotateRight|Move|RotateLeft|Move|Move|Move

Where move is a unit movement on a grid.

I did a very basic design in java. (Complete Code Pasted below)

package com.roverboy.entity;

import com.roverboy.states.RotateLeftState;
import com.roverboy.states.RotateRightState;
import com.roverboy.states.State;

public class Rover {

    private Coordinate roverCoordinate;
    private State roverState;

    private State rotateRight;
    private State rotateLeft;
    private State move;

    public Rover() {
        this(0, 0, Compass.NORTH);
    }

    public Rover(int xCoordinate, int yCoordinate, String direction) {
        roverCoordinate = new Coordinate(xCoordinate, yCoordinate, direction);
        rotateRight = new RotateRightState(this);
        rotateLeft = new RotateLeftState(this);
        move = new MoveState(this);
    }

    public State getRoverState() {
        return roverState;
    }

    public void setRoverState(State roverState) {
        this.roverState = roverState;
    }

    public Coordinate currentCoordinates() {
        return roverCoordinate;
    }

    public void rotateRight() {
        roverState = rotateRight;
        roverState.action();
    }

    public void rotateLeft() {
        roverState = rotateLeft;
        roverState.action();
    }

    public void move() {
        roverState = move;
        roverState.action();
    }
}


package com.roverboy.states;

public interface State {

    public void action();
}

package com.roverboy.entity;

import com.roverboy.states.State;

public class MoveState implements State {

    private Rover rover;

    public MoveState(Rover rover) {
        this.rover = rover;
    }

    public void action() {
        rover.currentCoordinates().setXCoordinate(
                (Compass.EAST).equalsIgnoreCase(rover.currentCoordinates()
                        .getFacingDirection()) ? rover.currentCoordinates()
                        .getXCoordinate() + 1 : rover.currentCoordinates()
                        .getXCoordinate());

        rover.currentCoordinates().setXCoordinate(
                (Compass.WEST).equalsIgnoreCase(rover.currentCoordinates()
                        .getFacingDirection()) ? rover.currentCoordinates()
                                .getXCoordinate() - 1 : rover.currentCoordinates()
                                .getXCoordinate());

        rover.currentCoordinates().setYCoordinate(
                (Compass.NORTH).equalsIgnoreCase(rover.currentCoordinates()
                        .getFacingDirection()) ? rover.currentCoordinates()
                                .getYCoordinate() + 1 : rover.currentCoordinates()
                                .getYCoordinate());

        rover.currentCoordinates().setYCoordinate(
                (Compass.SOUTH).equalsIgnoreCase(rover.currentCoordinates()
                        .getFacingDirection()) ? rover.currentCoordinates()
                                .getYCoordinate() - 1 : rover.currentCoordinates()
                                .getYCoordinate());
    }
}


package com.roverboy.states;

import com.roverboy.entity.Rover;

public class RotateRightState implements State {

    private Rover rover;

    public RotateRightState(Rover rover) {
        this.rover = rover;
    }

    public void action() {
        rover.currentCoordinates().directionOnRight();
    }

}

package com.roverboy.states;

import com.roverboy.entity.Rover;

public class RotateLeftState implements State {

    private Rover rover;

    public RotateLeftState(Rover rover)
    {
        this.rover = rover;
    }

    public void action() {
        rover.currentCoordinates().directionOnLeft();
    }

}


package com.roverboy.entity;

public class Coordinate {

    private int xCoordinate;
    private int yCoordinate;
    private Direction direction;
    {
        Direction north = new Direction(Compass.NORTH);
        Direction south = new Direction(Compass.SOUTH);
        Direction east = new Direction(Compass.EAST);
        Direction west = new Direction(Compass.WEST);
        north.directionOnRight = east;
        north.directionOnLeft = west;
        east.directionOnRight = north;
        east.directionOnLeft = south;       
        south.directionOnRight = west;
        south.directionOnLeft = east;
        west.directionOnRight = south;
        west.directionOnLeft = north;
        direction = north;
    }

    public Coordinate(int xCoordinate, int yCoordinate, String direction) {
        this.xCoordinate = xCoordinate;
        this.yCoordinate = yCoordinate;
        this.direction.face(direction);
    }

    public int getXCoordinate() {
        return xCoordinate;
    }
    public void setXCoordinate(int coordinate) {
        xCoordinate = coordinate;
    }
    public int getYCoordinate() {
        return yCoordinate;
    }
    public void setYCoordinate(int coordinate) {
        yCoordinate = coordinate;
    }

    public void directionOnRight()
    {
        direction.directionOnRight();
    }

    public void directionOnLeft()
    {
        direction.directionOnLeft();
    }

    public String getFacingDirection()
    {
        return direction.directionValue;
    }
}

class Direction
{
    String directionValue;
    Direction directionOnRight;
    Direction directionOnLeft;

    Direction(String directionValue)
    {
        this.directionValue = directionValue;
    }

    void face(String directionValue)
    {
        for(int i=0;i<4;i++)
        {
            if(this.directionValue.equalsIgnoreCase(directionValue))
                break;
            else
                directionOnRight();
        }
    }

    void directionOnRight()
    {
        directionValue = directionOnRight.directionValue;
        directionOnRight = directionOnRight.directionOnRight;
        directionOnLeft = directionOnRight.directionOnLeft;             
    }

    void directionOnLeft()
    {
        directionValue = directionOnLeft.directionValue;
        directionOnRight = directionOnLeft.directionOnRight;
        directionOnLeft = directionOnLeft.directionOnLeft;      
    }
}

Now my doubt is with this last class "Direction" and "Coordinate". coordinate represents a coordinate object for rover which helps it maintain its direction. Currently to keep track of direction I am using a doubly linked list of Direction objects, which pretty much work like a compass. Rotate left or right.

Here are the questions that I have. 1. I have used state pattern and shown design for direction tracking. Is there a better approach to simplify even this? Rem. I need to maintain coordinates correctly; such that if you move towards +y axis, my coordinates should be in + else in minus. Same for X axis.

2. Currently the responsibility for changing the face of the rover is indirectly delegated to coordinates and to direction class. Is this really correct? Isn't rover responsible for maintaining direction? Am I really right in my design to delegate that responsibility down to coordinate and direction class; just because it is easier to manipulate it there?

3. Any simple design improvements and suggestions on code will be most welcome. Feel free to critique.

Thanks for your patience and feedback; in advance.

Answer 1

It seems too complicated for me. I think it should be done in such a way: let your robot know his turning angle. Then if he is asked to turn left or right he will just change this angle. When he is asked to move he will move according to this angle in x,y coordinates. angle can be stored like compass or even simplier with real angle (0, 90, 180, 270). It is easy to move robot in angle direction by multiplying movement step on sin(angle) and cos(angle). Why cant it be that simple? It will also handle more directions that just 4 and youll be able to move in any step range.

Answer 2

My immediate thought upon looking at this is some confusion. The Rover class has 4 states and a direction, which seems a little counter intuitive. I would expect a position and a direction (for State I would, perhaps, expect, ON/OFF/RECHARGING or something similar).

So, I would investigate Java enums and have a NORTH/SOUTH/EAST/WEST Direction enum for the direction. The position (coordinate) has x/y positions, and to move, I would simply implement a deltaX() and deltaY() on the facing enumeration (it looks like Carl has just posted something similar)

Then your movement code would simply look like:

x += facing.deltaX()
y += facing.deltaY()

whichever direction you're facing. Note this doesn't delegate the movement. The Rover always moves, but the Direction enumeration gives it the dx/dy to change by.

The enumeration can also have methods clockwise() and counterClockwise(), so calling NORTH.clockwise() would return your new facing value EAST. Each enumeration instance would only have the delta and clockwise/counter-clockwise methods, and your Rover simply has the following:

private Direction facing;
private int x;
private int y;

which seems much more intuitive and what I'd expect. I've expressed x and y separately, but you may want to wrap in one class. If you do that, then the Direction enumeration should handle such an object, and not rely on it being broken apart again into x and y.

Answer 3

Here's a Direction enum I came up with the other day, of which I am perhaps inordinately fond. Perhaps you will find it useful in your code.

import java.awt.Point;

public enum Direction {
    E(1, 0), N(0, 1), W(-1, 0), S(0, -1);
    private final int   dy;
    private final int   dx;

    private Direction(int dx, int dy) {
        this.dx = dx;
        this.dy = dy;
    }

    public Direction left() {
        return skip(1);
    }

    public Direction right() {
        return skip(3);
    }

    public Direction reverse() {
        return skip(2);
    }

    private Direction skip(int n) {
        final Direction[] values = values();
        return values[(ordinal() + n) % values.length];
    }

    public Point advance(Point point) {
        return new Point(point.x + dx, point.y + dy);
    }
}

Answer 4

You're asking for how to simplify. If I may suggest something bold, why not use an opaque int for direction and have a static class to deal with it? By "opaque int" I mean that your code would never use it directly, but only as argument to the Direction class.

Here's some partial java-styled pseudocode to show what I mean.

// 0 = east, 1 = north, 2 = west, ...
public class Direction {
  static int [] moveX = [ 1, 0, -1, 0];
  static final int NORTH = 1;
  // coordinates after moving one step in the given direction
  static Pair move(int direction, Pair old) {
     return new Pair( old.x + moveX[direction] , old.y + moveY[direction] );
  }
  static int turnLeft(int direction) { 
     return (direction+1) % 4;
  }
  static int turnRight(int direction) {
     return (direction+3) % 4;
  }
}

This way of doing things would have the advantage of using fewer allocations, so the garbage collector won't need to run as often. Another advantage is that the design remains object-oriented in the sense that you can easily change the direction class if later you want to be able to rotate by e.g. 45 degrees at a time.

To answer your other questions, I think it's perfectly fine to delegate to the Direction class the task of changing a coordinate along a certain direction. The rover would be responsible for maintaining direction only in the sense that the rover object would contain an int field to store the direction it's facing.

Answer 5

The first thing What comes to my mind when I see this code is that Direction should not have a String field directionValue, but rather a field storing a Compass (i.e. Compass.EAST, Compass.WEST). This would get you rid of the String comparisons in MoveState.action() and should therefore make your code considerably cleaner.

There also seems to be a problem with naming: maybe NORTH, EAST, WEST, and SOUTH should be in an enum called Direction (instead of Compass), and directionOnRight() etc. in the current Direction implementation should be its static methods (getting the current direction as a single argument, and returning the right/left/reverse direction)? You don't really need to store them in extra fields IMHO (remember the saying about premature optimization ;-).