Predict mars robot position
$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?
java object-oriented programming-challenge
$endgroup$
add a comment |
$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?
java object-oriented programming-challenge
$endgroup$
add a comment |
$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?
java object-oriented programming-challenge
$endgroup$
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?
java object-oriented programming-challenge
java object-oriented programming-challenge
asked 2 hours ago
CodeYogiCodeYogi
2,01742875
2,01742875
add a comment |
add a comment |
2 Answers
2
active
oldest
votes
$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
34 mins ago
add a comment |
$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 theDirection
enum refactoring (+1), but for a future-friendly robot, I thinkenum Direction
is a bad idea. (See my answer.)
$endgroup$
– AJNeufeld
37 mins ago
$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 recalculatingx
andy
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
18 mins ago
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f214669%2fpredict-mars-robot-position%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
$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
34 mins ago
add a comment |
$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
34 mins ago
add a comment |
$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$
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;
answered 42 mins ago
AJNeufeldAJNeufeld
5,6041420
5,6041420
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
34 mins ago
add a comment |
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
34 mins ago
1
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
34 mins ago
$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
34 mins ago
add a comment |
$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 theDirection
enum refactoring (+1), but for a future-friendly robot, I thinkenum Direction
is a bad idea. (See my answer.)
$endgroup$
– AJNeufeld
37 mins ago
$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 recalculatingx
andy
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
18 mins ago
add a comment |
$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 theDirection
enum refactoring (+1), but for a future-friendly robot, I thinkenum Direction
is a bad idea. (See my answer.)
$endgroup$
– AJNeufeld
37 mins ago
$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 recalculatingx
andy
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
18 mins ago
add a comment |
$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$
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.
edited 23 mins ago
answered 50 mins ago
Konrad MorawskiKonrad Morawski
1,698710
1,698710
$begingroup$
Since this is a programming-challenge, I like theDirection
enum refactoring (+1), but for a future-friendly robot, I thinkenum Direction
is a bad idea. (See my answer.)
$endgroup$
– AJNeufeld
37 mins ago
$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 recalculatingx
andy
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
18 mins ago
add a comment |
$begingroup$
Since this is a programming-challenge, I like theDirection
enum refactoring (+1), but for a future-friendly robot, I thinkenum Direction
is a bad idea. (See my answer.)
$endgroup$
– AJNeufeld
37 mins ago
$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 recalculatingx
andy
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
18 mins ago
$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
37 mins ago
$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
37 mins ago
$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
18 mins ago
$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
18 mins ago
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f214669%2fpredict-mars-robot-position%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown