Python solution to Mars Rover

Posted on


I’m still quite new to Python, but am loving the language (I come from a more strongly-typed language background..).

A month or so ago I found the Mars Rover Challenge and attempted to solve it.
I quite quickly managed to solve the problem, but in all honesty I can’t help but feel like there is a lot of code-smells in my solution.

Could I get some help on ways to refactor functions and (if possible) add polymorphism to the classes to make it easier to extend on them?

Overall feedback would be great too!

#!/usr/bin/env python3

from Rover import Rover, directions
from Mars import Mars
import re

def validate_int(x, y):
    Validate that the two parameters are integers.
    Re-used multiple times during the execution.
        if int(x) >= 0 and int(y) >= 0:
            return True
    except Exception as err:
        print("Only numerical elements. Try again!")
        return False

def validate_rover(x, y, direction):
    Validates a rover to ensure the parameters are
    correct datatypes(int, int, string).
    It also controls ensures that the integers
    are inside the Mars surface and that the
    supplied direction is a correct direction.

        if validate_int(x, y) and direction in directions:
            return True
    except ValueError as err:
        print("Error: {}n. Please enter two numbers followed by a char either N, E, S or WnTry again!".format(err))
        return False
    print("You seem to have entered an incorrect Rover.nPlease enter two numbers followed by a char either N, E, S or WnTry again!n")
    return False

def validate_operations(op):
    Uses regex to validate that
    the supplied string only contains
    'M', 'R' and 'L'.

    Raises a ValueError if incorrect
    operation(s) have been supplied.
    pattern = re.compile("^[MRL]*$")

    if pattern.match(op):
        return True
        raise ValueError("Only values 'L', 'M' or 'R' accepted!")

def move(op, r):
    Uses the supplied operations
    and moves the rover according to
    the string of operations.

    If a rover goes out of bounds it is
    returned to its initial position
    (where it was initialized at).
        for operation in op:
            if operation == "L":
            elif operation == "R":
    except Exception as err:
        op = input("Error: {}nReturning it to inital position ({}, {} facing {}).
Try again!n>>> ".format(err, r.initial[0], r.initial[1], r.initial[2]))
        move(op, r)

def add_rover(mars):
    Taking a reference to Mars
    this function asks for user input.
    The user input is then validate.
    If it passes validation a new Rover
    is created and returned.
    If the input doesn't pass validation
    the user is prompted to enter a
    new Rover.
    while True:
        rover = None
            choice = check_if_exit(input("Please enter the current rover's initial position.nRemember to keep inside Mars limits!n>>> "), mars).split()
            if len(choice) == 3 and validate_rover(choice[0], choice[1], choice[2]): # Check the length of supplied input and check type-integrity
                rover = Rover(int(choice[0]), int(choice[1]), choice[2], mars)       # Initiate a rover with the supplied input. Each rover is assigned to a Mars (many-to-one relation).
        except ValueError as err:

        if rover is not None:
            return rover

def move_rover(rover, mars):
    Taking the created rover and mars
    the function then asks for user input.
    The input is validated and then
    sent to move() which then performs
    the operations.

    The Rover is then added to the
    occupied spaces on Mars and a
    new Rover will be prompted.
    while True:
        moved = False
            choice = check_if_exit(input("Enter a sequence of operations.n>>> "), mars).upper()
            if validate_operations(choice): # Validate that the supplied operations are 'L', 'M' or 'R'
                move(choice, rover)         # Peform the moves on the rover
                mars.occupied.append((rover.x, rover.y, rover.direction))
                moved = True
        except Exception as err:

        if moved:

def go_end(mars):
    This function simply gives a pretty output
    of all the Rovers on Mars surface.
    Then exits the application.
    print("--------------- Output ---------------")
    for rover in mars.occupied:
        print("{} {} {}".format(rover[0], rover[1], rover[2]))
    print("Exiting application.")

def check_if_exit(iput, mars):
    Every input taken after setting up Mars will
    use this function to check if the input
    contains 'exit'.

    If it contains 'exit' it the go_end
    function will be called.
    if "exit".upper() in iput.upper():
    return iput

def main ():

    The brains of the code.

    Uses a few while loops to validate
    user input and to make the code more user friendly.

    An exit when prompted will print the rover's positions.

    inactivate_loop = False

    while (not inactivate_loop): # will loop until Mars has been initialized.
        choice = input("Please enter the size of mars (2 numbers, separated by a space and higher than 0.)n>>> ").split()

        if len(choice) == 2 and validate_int(choice[0], choice[1]): # Checks so that the input is length 2 and then checks type-integrity
            mars = Mars(int(choice[0]), int(choice[1]))
            inactivate_loop = True
            print("Incorrect input. Please ensure you enter a string with two numerical elements")

    while True:
            rover = add_rover(mars)       # Initiate a rover with the supplied input. Each rover is assigned to a Mars (many-to-one relation).
            move_rover(rover, mars)

if __name__ == "__main__":

class Mars(object):
    As mentions above, creates a Mars-object.
    def __init__(self, x, y):
        Initializes a Mars-object.
        self.x = x
        self.y = y
        self.occupied = []

The Rover class handles everything concerning the rover.
Meaning it handles the initialization of new rovers,
the movement and resetting of it.

A rover is initialized with two integers (its x- and y-values)
a direction (N, E, S, W) and what Mars it is related to (a many-to-one relationship).

The reason I added Mars to the initialization is mainly due to the fact
that it would be easy to add multiple rectangles if that was in the test-case
and at the same time move a Rover from one Mars to another.

It also makes it easier when validating the Rover to its assigned Mars as
we can ensure that it is in that particular Mars size.

I also decided to use the operator library as I wanted to show
that I understand the concept of working with getters and setters.
Even though that isn't necessary to use in Python it does make
the validation of attributes a bit 'neater' in my opinion.

import operator
directions = ("N", "E", "S", "W") # Use tuple since we don't need to manipulate
 # the direciton after initializing it (immutable)
class Rover(object):
    As mentioned above, creates a Rover-object.
    def __init__(self, x, y, direction, mars):
        Initialize a rover
        self.Mars = mars
        self.direction = direction
        self.x = x
        self.y = y
        self.initial = (self._x, self._y, self._direction)

    direction = property(operator.attrgetter('_direction'))

    Setters, uses basic validation to ensure type-integrity.
    In directions uses tuple to ensure that direction is indeed correct.
    def direction(self, d):
        Checks if the direction exists in the
        defined directions.
        If the direction doesn't exist a
        ValueError is raised.
        Otherwise the direction is set.
        if d.upper() not in directions:
            raise ValueError("Direction not correct, use 'N, E, S or W'")
        self._direction = d.upper()

    x = property(operator.attrgetter('_x'))

    def x(self, x):
        Checks if x is within acceptable
        bounds (higher than 0 and lower than
        Mars surface)
        if (x < 0 or x > self.Mars.x):
            raise ValueError("""This rover's x-value is out of bounds.
It should be value should be < 0 and > {}""".format(str(self.Mars.x)))
        self._x = x

    y = property(operator.attrgetter('_y'))

    def y(self, y):
        Checks if y is within acceptable
        bounds (higher than 0 and lower than
        Mars surface).
        if (y < 0 or y > self.Mars.y):
            raise ValueError("This rover's y-valueis out of bounds.
It should be value should be < 0 and > " + str(self.Mars.y))
        self._y = y

    initial = property(operator.attrgetter('_initial'))
    def initial(self, tup):
        Checks if the two first items in the
        passed tupple tup exist
        in the occupied spaces in the rover's
        assigned Mars.
        If it does exist that means the
        space is already taken and an error
        is raised.
        If the space is empty, the space is
        set to the initial space (used
        if the Rover goes out of bounds).
        for spaces in self.Mars.occupied:
            if (tup[0], tup[1]) == (spaces[0], spaces[1]): raise RuntimeError("This position is 
already occupied by another rover")
        #if (tup[0], tup[1]) in self.Mars.occupied: raise IndexError("This position is already occupied by another rover")
        self._initial = tup

    def get_formatted_position(self):
        Returns a formatted string containing the
        Rover's position (as used in output).
        return "{} {} {}".format(self._x, self._y, self._direction)

    def get_current_position(self):
        Get the current position of the rover.
        Usually we don't need getters in Python,
        but I wanted to return a formatted string.

        This is also used in the unittest-cases to
        assert the position of the Rover.
        return (self._x, self._y)
    Re-initializes the object's positioning
    in case the rover goes out of bound.
    def return_to_start(self):
        self.x = self._initial[0]
        self.y = self._initial[1]
        self.direction = self._initial[2]

    Movement functions used to move the rover.
    def turnRight(self):
        Checks if the current direction is in the
        end of the tuple.
        If it's in the end of the
        tuple we take the first item in the tuple
        to be the new direction.
        Otherwise we take the element on index + 1.
        Basically attempts to mimic a compass by
        simulating a 'circular' list.
        self.direction = directions[0] 
        if directions.index(self._direction) == 3 
        else directions[directions.index(self._direction) + 1]

    def turnLeft(self):
        Does the opposite of turnRight
        by checking if the item is in
        the beginning of the list.
        self.direction = directions[3] 
        if directions.index(self._direction) == 0 
        else directions[directions.index(self._direction) - 1]

    def forward(self):
        Move the Rover forward in the direction it is facing
        by setting its Y- or X-value.
        This will use the class setters, so if the
        Rover goes outside of bounds it will raise an
        exception, which in turn will reset its position!

        It also raises an exception in case a forward Movement
        means it hits another rover already occupying its
        new space.
        if self._direction == "N":
            self.y = self._y + 1
        elif self._direction == "S":
            self.y = self._y - 1
        elif self._direction == "E":
            self.x = self._x + 1
            self.x = self._x - 1
        #Test the current space
        for spaces in self.Mars.occupied:
            if self.get_current_position() == (spaces[0], spaces[1]):
                raise RuntimeErrorjk("I've hit a position that is already taken.n
Returning to my initial position. Please try again!n")


For once it is nice to see code which is properly commented and for the most parts is easy to read and follow. Kudos to you for that. However, there are stuff which I’m wondering on why you’ve done as you’ve done, and with possible suggestions as to how you could tackle those.

Starting from the top:

  • Validate input, not necessarily all parameters – Parameters you pass around would most likely not change by itself to something illegal. As such, it is most common to have the try...except closer to the input.

    In your code, you’re having try...except both in the actual input method, and also within the actual input loops. Wouldn’t it be better to have a dedicated input method which did both cases?

  • Maybe overly fond of try...except? – In general we don’t see a lot of try...catch in the code up for review, so it is nice to see that someone is actually thinking about error handling. It does feel like you’ve taken it a little long, though, and that you need to take it back a step. This is more of a general feeling though, and could be mostly personal opinion.

  • Consider to have the x, y, direction in a dedicated structure – This could possibly benefit from its own class (or structure) just to ease passing it around, and referencing it.

    That would also alleviate the need for using the initials array and choice array in some parts of your code.

  • Feature: Potential loop in move(op, r) – If the rover triggers the exception you move it back to start, and start moving again (recursively). What stops it from triggering error another time? And again? And again? And …

  • Why mars and not planet or plane? – It confuses me when I read about multiple mars‘es… :-/

  • Doing input within exception handling is not good – In general, you should avoid doing new input within exception handling as that might trigger other exceptions. The exception handling should be kept to a minimum to clear out the current error situation, and allow for normal operations to preceed (or bail out due to the severity of the exception).

  • Docstring’s could be on fewer lines – It’s good to have docstrings,
    but you take advantage of all the 80 characters available (according to standards), as then they wouldn’t take up as much space.

  • The add_rover doesn’t actually add a rover – Name methods after what they actually do. This method gets a new legal position and direction for a rover, and doesn’t add it anywhere.

    It could possibly be named get_rover_initial_position or something like that.

  • Strange logic in add_rover – Having a while True in combination with a return of if rover is not None?, and a possible exit through the check_if_exit method… My head is spinning trying to follow the logic, and it really shouldn’t be that hard. Imaging something like:

    def get_new_rover(planet):
        """Get a valid position and direction within the planet."""
        rover = None
        while not Rover:
            x, y, direction = get_initial_position("Please enter current rover's initial position and direction");
                rover = Rover(x, y, direction, planet);
            exception PlanetError as err:
        return rover

    It is better, but I still think I would prefer to validate the position instead of triggering an exception within Rover. And I left out the exit handling, which could be done through raising another exceptions or at a higher level or other means. I’ve got to come back to this one…

  • Avoid flag variables, especially in combination with While true – You use this pattern a few times:

    While True:
         flag = False
         ... do something possibly setting: flag = True 
         if flag:

    A much better pattern is the following:

    flag = False
    While not flag:
         ... do something possibly setting: flag = True 

    And it would be even better if you actually used the parameter you’re trying to set instead of a whimsical flag parameter. See rover example above.

  • move_rover does a lot more than just moving the rover – It gets a new operation sequence, tries to move it, updates the planet trail/occupied list and prints a line…

  • Regarding go_end – This method by name goes to the end. It then proceeds with outputting the trail of the rovers, and boom exits. I’ve never liked a hidden exit in the middle of the code, and would much prefer it to be visible at a much higher level.

  • Why "exit".upper()? – As said before, I’m not entirely thrilled about the the check_if_exit() as it suddenly terminates the program. But why do you not simply write “EXIT” instead of doing it over and over again? (Most likely the compiler will remove this, but it looks a little strange)

Some refactoring

Based upon all of this I would rewrite your main method to something like the following:

# Create a planet to move the rovers in
x, y = input_planet_size()
planet = Planet(x, y)

while True:
    x, y, direction = input_position_and_direction(planet) 

    # Use a negative x coordinate to trigger exit
    if x < 0:

    rover = Rover(x, y, direction)

    commands = input_commands()

    if commands.lower() == "exit":



I’ve introduced some new methods here, which in my book would make sense to move the program forward. The input_xxxx methods are in charge of validating the input, and an indicator to exit everything. Here is an alternative for input_planet_size():

def input_planet_size():
  """Read and return width and height, or raise SystemExit."""

  while True:
    text = input("Please enter width and height (or 'exit'): ")

    if text.lower() == "exit":
      raise SystemExit 

    # Validate that the input is actually int's       
      width, height = map(int, text.split()[:2])
    except ValueError:
      print("Those were not numbers (nor 'exit'). Try again")

    # Continue validation of numbers
    if width > 0 and height > 0:
      return width, height

w, h = input_planet_size()
print("Hooray, I got ({}, {})".format(w, y))

Two notes on this last piece of code:

In most cases I would prefer to either return a value indicating the exit condition, but using raise SystemExit could also work nicely as it triggers the proper cleanup.

Update: new forward method

Based upon commments, I suggested using a dictionary with the direction as key, and tuples for x and y movement as values. Here is some code demonstrating this principle:

DIRECTIONS = {'N': (0, 1), 'E': (1, 0), 'S': (0, -1), 'W': (-1, 0)}

x = 0
y = 0
print("Start – X: {}, Y: {}".format(x, y))

for direction in "NESWNNEEE":
  x += DIRECTIONS[direction][0]
  y += DIRECTIONS[direction][1]
  print("{} -> X: {}, Y: {}".format(direction, x, y))

print("End – X: {}, Y: {}".format(x, y))

Or to implement it directly into the forward method:

def forward(self):

    self.x += DIRECTIONS[self._direction][0]
    self.y += DIRECTIONS[self._direction][1]

    if (self.x, self.y) in self.Mars.occupied:
        raise RuntimeErrorjk("Taken... Try again")

And that is rather neat, if I must say so… 🙂

Leave a Reply

Your email address will not be published. Required fields are marked *