Mars rover movement and position

Question

I have written a solution of a Mars Rover problem in C++ using OOP, but I am not fully satisfied with the design. What changes can I make to improve the design?

Problem description is:

A rover’s position and location is represented by a combination of x and y co-ordinates and a letter representing one of the four cardinal compass points. The plateau is divided up into a grid to simplify navigation. An example position might be 0, 0, N, which means the rover is in the bottom left corner and facing North.

In order to control a rover , NASA sends a simple string of letters. The possible letters are ‘L’, ‘R’ and ‘M’. ‘L’ and ‘R’ makes the rover spin 90 degrees left or right respectively, without moving from its current spot. ‘M’ means move forward one grid point, and maintain the same heading.

#include<iostream>
#include<string>
#include<fstream>
using namespace std;

class Rover{

int x;
int y;
char orientation ;

public :

Rover();
Rover( int , int , char );
void rotateLeft();
void rotateRight();
void movePosition();
void displayPosition();

};    

Rover :: Rover()
{
x =0;
y =0;
orientation = 'N';

}

Rover :: Rover ( int positionX , int positionY , char Orientation )
{
x= positionX;
y= positionY;

orientation = Orientation ;
}

void Rover::  displayPosition()
{
cout<<x<<" "<<y<<" "<<orientation;
}

void Rover :: rotateLeft()
{
 if(orientation=='N')
 orientation= 'W'; 
 else if(orientation=='W')
 orientation='S';
 else if(orientation=='S')
 orientation='E';
 else if(orientation=='E')
 orientation='N';

}

void Rover :: rotateRight()
{
 if(orientation=='N')
 orientation= 'E'; 
 else if(orientation=='W')
 orientation='N';
 else if(orientation=='S')
 orientation='W';
 else if(orientation=='E')
 orientation='S';

}

void Rover ::movePosition()
{
 if(orientation=='N')
    y++;
 else if(orientation=='W')
    x--;
 else if(orientation=='S')
    y--;
 else if(orientation=='E')
    x++;
}


int main()
{

int x,y; char orient ;
cin>>x>>y>>orient;
Rover firstRover(x,y,orient);

firstRover.displayPosition();

string roverMovement;
cin>>roverMovement;

for(int i=0 ; i < roverMovement.size() ; i++  )
{

     if(roverMovement[i]=='L')
     firstRover.rotateLeft();

     else if(roverMovement[i]=='R')
     firstRover.rotateRight();

     else
     firstRover.movePosition();
}

firstRover.displayPosition();

return 0;
}

How the design of this code can be improved?

Answer 1

The design is fine. You modeled the rover as a class, its actions with methods, and its state with member variables. This is natural, intuitive and simple, good this way.

On top of the issues Jerry already pointed out in his answer, I have a few additional comments about the implementation technique.

The rotation methods are a bit tedious, and tedious tasks are sometimes error prone. I would recommend a different approach for changing the orientation:

• Create a simple string with the orientations in clockwise order, that is "NESW"
• Forget about storing the name of the current orientation, it's enough to store its index in the orientations string. In all actions of the rover, we can work with this index and the string
• When rotating right, the index of the target orientation is the index of the current +1 modulo length(orientations)
• When rotating left, the index of the target orientation is the index of the current + length(orientations) -1 modulo length(orientations)

For moving forward, you could do similarly, by using arrays for the x and y movements depending on the index of the current orientation, so for x the content will be {0, 1, 0, -1}, and for y it will be {1, 0, -1, 0}.

The benefit of this approach will be faster actions, as there will be no more conditional statements (also called a table driven approach).

Answer 2

Indentation

You need it. Badly. Until you fix it, essentially everything else is irrelevant.

Prefer member initializer lists to assignment in the constructor

For example, I'd the default constructor written something like this:

Rover :: Rover()
    : x(0)
    , y(0)
    , orientation('N')
{ }

There's nothing wrong with a ctor having an empty body. In fact, I consider it pretty much the norm.

Only define functions that add value

If you didn't define a copy constructor, the compiler would define one for you that did exactly the same (member-wise copying). In such a case, it's better to let the compiler do the job.

For case-like situations, prefer a case statement

For your rotateLeft function, I'd write the code more like this:

void Rover :: rotateLeft()
{
    switch (orientation) { 
    case 'N': orientation = 'W'; break;
    case 'W': orientation = 'S'; break;
    case 'S': orientation = 'E'; break;
    case 'E': orientation = 'N'; break;
}

The rotateRight and movePosition functions could follow a similar pattern.

Answer 3

Overall design is good for the problem you are solving & the assumptions you've made.

Some Suggestions:

• Create an enum for Orientation (with values NORTH, SOUTH, EAST, WEST) instead of using char. It will allow you to extend easily in the future if say North-East (NE), SW, NW, SW get added

• As already suggested above, use switch instead of nested if-else, it will run slightly faster. Switch can be used on any integral type (char, enum, int)

• Get rid of the default constructor for 'Rover' (which initializes to 0,0,N). It may lead to incorrect usage. A 'Rover' instance should always know its position. Force caller to always create one with some initial position.

• Make displayPosition a const member function as it doesn't change any member values

Minor/Stylistic things:

• Indentation (this is a matter of style, but follow some popular opensource projects or C/C++ author). Develop a habit, will help you in the long run

• I prefer checking conditions as 'N' == orientation (i.e. const being compared to a variable), that way compiler will help catch typos in case you had single '=' instead of '=='

• Remove using namespace std; from global scope. It is a bad practice. If you want to avoid typing, add it in the innermost function scope that needs it.