5
\$\begingroup\$

Description

A robot lands on Mars, which happens to be a cartesian grid; assuming that we hand the robot these instructions, such as LFFFRFFFRRFFF, where "L" is a "turn 90 degrees left", "R" is a "turn 90 degrees right", and "F" is "go forward one space, please write control code for the robot such that it ends up at the appropriate-and-correct destination, and include unit tests.

Here is an example output with command "FF":

[0, 2]

Code

class Robot {
  private int x;
  private int y;
  private int currentDirection;

  Robot() {
    this(0, 0);
  }

  Robot(int x, int y) {
    this.x = x;
    this.y = y;
    currentDirection = 0;
  }

  public void move(String moves) {
    for (char ch : moves.toCharArray()) {
      if (ch == 'R') currentDirection += 1;
      if (ch == 'L') currentDirection -= 1;

      currentDirection = currentDirection % 4;

      if (ch != 'F') continue;

      System.out.println(currentDirection);
      if (currentDirection == 0) {
        y += 1;
      } if (currentDirection == 1) {
        x += 1;
      } if (currentDirection == 2) {
        y -= 1;
      } if (currentDirection == 3) {
        x -= 1;
      }
    }
  }

  public void reset() {
    x = 0;
    y = 0;
    currentDirection = 0;
  }

  public String position() {
    return x + ":" + y;
  }
}

class Main {
  public static void main(String[] args) {
    Robot robot = new Robot();
    robot.move("FF");
    System.out.println(robot.position()); // 0,2
    robot.reset();
    System.out.println(robot.position()); // 0,0
    robot.move("FFRF");
    System.out.println(robot.position()); // 1,2
    robot.reset();
    robot.move("FFRRRFF");
    System.out.println(robot.position()); // -2,2
  }
}

How can I make this code more object oriented?

\$\endgroup\$
7
\$\begingroup\$

Initialization

As written, your robot lander can land at any X,Y coordinate on your grid, but will always be facing in the positive Y-axis direction. This seems unreasonable. If wind, turbulence can cause a position uncertainty, which requires initializing the robot at a particular X,Y coordinate, it seems reasonable to assume it might also land in any facing.

Robot() {
   this(0, 0, 0);
}

Robot(int x, int y) {
    this(x, y, 0);
}

Robot(int x, int y, int initial_facing) {
    // ...
}

Commands & Instructions

Your instructions are a series of single letter commands, but you can't actually send a single command to the robot. You should separate the individual commands from the series of instructions. Something like:

void turn_left() { ... }
void turn_right() { ... }
void move_forward() { ... }

public void command(char command_letter) {
    switch (command_letter) {
        case 'L': turn_left(); break;
        case 'R': turn_right(); break;
        case 'F': move_forward(); break;
        default: throw new IllegalArgumentException("Invalid command: "+command_letter);
    }
}

public void instructions(String moves) {
    for (char command_letter : moves.toCharArray()) {
        command(command_letter);
    }
}

Where am I?

position() returns a String containing the coordinates of the robot. If a control program wants to query the position of the robot, in order to determine what commands should be sent to send it to the desired location, it would need to parse that string back into integer values.

Consider instead returning the actual integer positions, possibly like:

public int[] position() {
    return new int[]{ x, y };
}

Alternately, you might want to create a class Position which can store the x,y location of the robot. Or you could use java.awt.Point

public Point position() {
    return new Point(x, y);
}

Perhaps override the toString() method to return a human friendly description of the robot, including its position. Or maybe a position_as_string() method.

Which way is the robot facing? Can't directly tell! You currently have to query the position(), then move("F"), followed by position(), and then compare the positions to determine which way the robot is facing! How about adding a facing() method?


Learn how to write proper unit tests. For example, with JUnit5, you could write:

import static org.junit.jupiter.api.Assertions.assertEquals;
import org.junit.jupiter.api.Test;

class RobotTests {

    private final Robot robot = new Robot();

    @Test
    void starts_at_origin() {
        assertEquals("0:0", robot.position());
    }

    @Test
    void move_forward_twice() {
        robot.move("FF");
        assertEquals("0:2", robot.position());
    }

    @Test
    void move_and_turn_right() {
        robot.move("FFRF");
        assertEquals("1:2", robot.position());
    }

    @Test
    void three_rights_make_a_left() {
        robot.move("FFRRRFF");
        assertEquals("-2:2", robot.position());
    }

    @Test
    void but_one_left_does_not() {
        robot.move("FFLFF");
        assertEquals("-2:2", robot.position());
    }
}

Notice that each test is run in a brand-new RobotTests instance, so you don't need to call robot.reset() between each.

If you run this unit test, you'll find 4 of 5 tests pass, one test fails. I'll leave you to figure out why.


Additional Concerns

currentDirection taking on the values 0, 1, 2 and 3 to represent the 4 cardinal directions is limiting. If later, you want to add in diagonal moves (NW, SW, SE, or NE), would you use the values 4 and above to represent them? Or would you renumber the original 4 directions to be 0, 2, 4 and 6, and use 1, 3, 5, and 7 for the diagonal directions?

You might be tempted to use an enum for the direction values, but I think that would be a bad idea. I would use 0, 90, 180, and 270 as direction values. These have physical meaning. If later your robot is allowed a more realistic double x, y; coordinate system, you could also change to use double currentDirection; and allow turns of a fraction of a degree. With enum, you'd lose this possible future flexibility.

As an alternative, you might also consider using a directional vector:

int dx=0, dy=1;   // currentDirection = 0°

And move_forward simply becomes:

x += dx;
y += dy;

And a turn right could become:

int old_dx = dx;

dx = dy;
dy = -old_dx;
\$\endgroup\$
  • 1
    \$\begingroup\$ This is solid advice. Personally I don't think an enum is a bad solution if we can assume the requirements aren't going to change. \$\endgroup\$ – Konrad Morawski Mar 3 at 23:14
7
\$\begingroup\$

I would consider refactoring direction into an enum, instead of a magic number. This would allow to encapsulate the related logic in this type.

It might look like that:

    enum Direction {
        UP(0, 1),
        RIGHT(1, 0),
        DOWN(0, -1),
        LEFT(-1, 0);

        final int xVector;
        final int yVector;

        private final Direction FIRST = Direction.values()[0];
        private final Direction LAST = Direction.values()[Direction.values().length];

        Direction(int xVector, int yVector) {
            this.xVector = xVector;
            this.yVector = yVector;
        }

        Direction rotatedLeft() {
            return this == FIRST
                    ? LAST // cycle complete
                    : Direction.values()[ordinal() - 1]; // previous value
        }

        Direction rotatedRight() {
            return this == LAST
                    ? FIRST // cycle complete
                    : Direction.values()[ordinal() + 1]; // next value
        }
    }

And then the Robot code becomes (including some subjective clean up, unrelated to the object-oriented aspect itself):

class Robot {
    private int x;
    private int y;
    private Direction currentDirection;

    Robot() {
        this(0, 0);
    }

    Robot(int x, int y) {
        this.x = x;
        this.y = y;
        currentDirection = Direction.UP;
    }

    public void move(String moves) {
        for (char code : moves.toCharArray()) {
            // a matter of taste, I like to extract logic 
            // out of looping constructs for clarity
            move(code);
        }
    }

    private void move(char code) {
        switch (code) {
            case 'R': {
                currentDirection = currentDirection.rotatedRight();
                break;
            }
            case 'L': {
                currentDirection = currentDirection.rotatedLeft();
                break;
            }
            case 'F': {
                // you'd call the println thing here.
                // as it's not part of the requirements I assumed it to be a debugging artifact
                this.x += currentDirection.xVector;
                this.y += currentDirection.yVector;
                break;
            }
        }
    }

    public void reset() {
        x = 0;
        y = 0;
        currentDirection = Direction.UP;
    }

    public String position() {
        return x + ":" + y;
    }
}

I think this is more object-oriented, and also an improvement.

You could take it one step further and encapsulate the x and y coordinates in a little standalone class (Position). Like so:

    class Position {
        static final Position DEFAULT = new Position(0, 0);

        final int x;
        final int y;

        public Position(int x, int y) {
            this.x = x;
            this.y = y;
        }

        public Position movedInto(Direction direction) {
            return new Position(x + direction.xVector, y + direction.yVector);
        }

        @Override
        public String toString() {
            return x + ":" + y;
        }
    }

Then in the Robot class you replace x and y fields with Position position, and you no longer recalculate the position in the Robot class, as you can simply go:

case 'F': {
    position = position.movedInto(currentDirection);
    break;
}

in the move method.

Plus:

    public void reset() {
        position = Position.DEFAULT;
        currentDirection = Direction.UP;
    }

    public String position() {
        return position.toString();
    }

Yet another idea would be to encapsulate the move codes themselves into a class or classes. It's sometimes hard to say where to draw the line, as making code more and more OO comes at the cost of more boilerplate, and can be a form of overengineering.

I admit I haven't tested my refactored version, I only approached the question in terms of code design.

\$\endgroup\$
  • \$\begingroup\$ Since this is a programming-challenge, I like the Direction enum refactoring (+1), but for a future-friendly robot, I think enum Direction is a bad idea. (See my answer.) \$\endgroup\$ – AJNeufeld Mar 3 at 23:11
  • \$\begingroup\$ Fixed the bug, thanks. The azimuth / alpha (I guess we could call it that?) is a neat alternative to an enum. An alpha value is more flexible as you pointed out, the cost is that recalculating x and y when the direction is, say, 270, won't look as straightforward. So I feel it's a tradeoff between explicity and flexibility. \$\endgroup\$ – Konrad Morawski Mar 3 at 23:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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