Mars Rover Simulator

Problem Statement


Consider a rover and a plateau of size nxn. The rover takes three type of instructions L,R and M. 'L' and 'R' rotate the rover in the left and right direction. 'M' moves the rover one step forward in the direction it faces.


Input: Plateau size, Initial direction, Initial position and Instructions.
Output: Final position and direction the rover faces.


Eg. Input: Plateau Size :5
Initial Direction : N
Initial Position : 0,0
Instructions: RMML


Output: 2,0 N.

I have written a program that does this. I would like a review on it. Also I have used a grid coordinate system to refer the plateau here. Is there a better way to do it ??

MarsRover.java

```
/**
* Created by Leo on 18-07-2015.
Completed on 23-07-2015
*/

import java.util.Scanner;

public class MarsRover {

private String inst_set;
private Plateau p;
private Cell currentLocation;

public enum Direction{N,S,E,W}

Direction dir;

public MarsRover(Plateau p1, int x, int y, Direction dir, String instSet) {

p=p1;
inst_set=instSet;
this.dir=dir;
currentLocation=new Cell(x, y);

}

public MarsRover(){}

private void executeInst() {

int i = 0;
while (i < inst_set.length()) {

char inst = inst_set.charAt(i++); //get each inst separately as a character

if (inst == 'M') {

Cell nextCell = p.getNeighbour(dir, currentLocation);

if (nextCell != null)
currentLocation = nextCell;

else
System.out.println("This move is not possible. Going to next Instruction");

}

if (inst == 'L' || inst == 'R') {

dir = setDirection(dir, inst);

}
}
}

public void showCurrentLocation(){

System.out.println("Current Cell is:");
System.out.print(currentLocation.x);
Sy

Form/Style

1. Format

In practice, your colleagues will appreciate it if you pay more attention to code formatting. Look into Checkstyle and commonly accepted conventions, such as those defined by Google.

1. Use good variable names.

m is not a good name for a MarsRover, and neither is mr1. Ignoring the fact that these variable names are short and nondescript, the way you abbreviate is inconsistent.

Per, this.dir = dir, you know the "this" keyword. There is a reason this exists, so use it. Now you don't have to take the time to figure out if "size" is sz or size. Just do the following:

public Plateau(int size){
    this.size = size;
}

The same applies to instSet (the constructor parameter) and inst_set (the instance variable [even though you use camelCase format for your other instance variables]) as well as many other variables throughout your code.

You will thank yourself later for using good variable names (even in stub/temporary code). You might not expect anyone to look at this code, but you did post it on Code Review SE.

1. And...

Instead of:

String dir;
dir = in.nextLine();

Just String dir = in.nextLine();.

Function

1. Don't make assumptions, particularly when it comes to user input.

"Enter the Size of the plateau"

How? Is "1x2" valid? How about "3.5?" If you are expecting an integer that describes the length of a square plateau, say so, and better yet, provide an example! Don't assume that the user knows what input is expected.


"Enter the Instructions\t"

Same as above. Also, "\t" is not needed.

1. On the subject of prompts...

"This move is not possible. Going to next Instruction"

Which move? This error message does not help the user fix the problem.

1. Branch properly and validate user input. Take, for example,

if (inst == 'M') { ... } if (inst == 'L' || inst == 'R') { ... }

Now look at the following:

if (inst == 'M') { ... }
else if (inst == 'L' || inst == 'R') { ... }
else { throw new IllegalArgumentException("bad instruction " + inst); }

1. main() does not belong in MarsRover. Put it in a separate class.

1. Use JUnit assertion messages.

MarsRover marsRover;
final MarsRover.Direction NORTH = MarsRover.Direction.N;
...
@Before
public void initialize() {
    marsRover = new MarsRover(new Plateau(5), 1, 1, NORTH, "");
}
@Test
public void testRotation() {
    MarsRover.Direction[] startDirection = new MarsRover.Direction[]{NORTH, EAST, SOUTH, WEST, NORTH, WEST, SOUTH, EAST};
    char[] instSet = new char[]{'R', 'R', 'R', 'R', 'L', 'L', 'L', 'L'};
    for (int i = 0; i < instSet.length; i++) {
        MarsRover.Direction endDirection =  startDirection[(i + 1) % startDirection.length];
        MarsRover.Direction actualDirection = marsRover.setDirection(startDirection[i], instSet[i]);
        assertEquals(instSet[i] + " of " + startDirection[i] + " should be " +
                endDirection + " but is " + actualDirection, endDirection, actualDirection);
    }
}

1. And...

// more work & less readable
private void executeInst() {
    int i = 0;
    while (i < inst_set.length()) {
        char inst = inst_set.charAt(i++);
        ...
// less work & more readable
private void executeInst() {
    for (char inst: instSet.toCharArray()) { ...

StackExchange Code Review Q#131538, answer score: 2