9
\$\begingroup\$

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?

\$\endgroup\$
  • 2
    \$\begingroup\$ I try to avoid writing code using the "oops" design. It tends not to work in the long run. :) \$\endgroup\$ – Michael Brandon Morris Jun 29 '16 at 14:52
  • 1
    \$\begingroup\$ Interesting aside: back in the 1980's (or so) a lot of kids were initially exposed to programming with a system very similar to this. The Logo programming languages included a "turtle" that worked similarly, though it did have a little more precision than this (e.g., turns could be arbitrary angles, not just 90 degrees). In most cases, the turtle was an abstraction that drew figures on the screen, but at MIT (I believe it was MIT, anyway) they had some physical ones that actually moved around a room. \$\endgroup\$ – Jerry Coffin Jun 29 '16 at 16:32
  • 2
    \$\begingroup\$ I would rename movePosition into something like moveForward. Also, I would never use bracket-less if blocks. \$\endgroup\$ – njzk2 Jun 29 '16 at 19:15
  • \$\begingroup\$ Work on formatting your code a bit more \$\endgroup\$ – Meme myself and a very creepy Nov 28 '18 at 2:31
8
\$\begingroup\$

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).

\$\endgroup\$
  • \$\begingroup\$ You said "river" and I think it should be "rover" (and it's too few characters for me to submit an edit). \$\endgroup\$ – Fuhrmanator Jun 29 '16 at 20:12
  • \$\begingroup\$ Instead of a string, he could use an enum, which would allow for a switch statement and maybe make it cleaner. \$\endgroup\$ – Meme myself and a very creepy Nov 28 '18 at 2:32
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Awesome suggestions . About the indentation it got distorted during adding code here and i will try better next time . thanks man . \$\endgroup\$ – rahul goyal Jun 29 '16 at 15:07
  • 2
    \$\begingroup\$ @rahulgoyal To avoid the problem in the future, just paste the code directly into the question, then select the code and type CTRL+K. \$\endgroup\$ – Justin Jun 29 '16 at 16:56
  • \$\begingroup\$ okay , got it (Y) . \$\endgroup\$ – rahul goyal Jun 29 '16 at 21:50
  • \$\begingroup\$ While I agree that reasonable indentation is important "Until you fix it, essentially everything else is irrelevant" is not really very productive feedback. Especially as the first line of a review \$\endgroup\$ – Harald Scheirich Sep 28 '17 at 11:58
2
\$\begingroup\$

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.

\$\endgroup\$

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.