7
\$\begingroup\$

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!


main.py

#!/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.
    """
    try:
        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.
    """
    try:

        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 W\nTry 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 W\nTry 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
    else:
        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).
    """
    try:
        for operation in op:
            if operation == "L":
                r.turnLeft()
            elif operation == "R":
                r.turnRight()
            else:
                r.forward()
    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]))
        r.return_to_start()
        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
        try:
            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:
            print(err)
            continue

        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
        try:
            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))
                #print(mars.occupied)
                moved = True
                print("\n--------------------------------------\n")
        except Exception as err:
            print(err)
            continue

        if moved:
            return

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("--------------------------------------")
    print("Exiting application.")
    exit()

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():
        go_end(mars)
    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
        else:
            print("Incorrect input. Please ensure you enter a string with two numerical elements")
    print("\n--------------------------------------\n")

    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__":
    main()

Mars.py:


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 = []

Rover.py

"""
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.
    """
    @direction.setter
    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'))

    @x.setter
    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'))

    @y.setter
    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'))
    @initial.setter
    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
        else:
            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")
                self.return_to_start()
\$\endgroup\$
  • \$\begingroup\$ Could you explain what "mars" is? It's clearly not the plane? And could you edit in a link to the "mars rover challenge"? \$\endgroup\$ – holroy May 10 '17 at 13:45
  • \$\begingroup\$ Mars is actually the plane. It basically sets up a gameboard object, or similar (ie. sets the x and y coordinates of a board where top-right is x,y and bottom-left is 0,0) veerasundar.com/blog/2010/06/mars-rover-in-java \$\endgroup\$ – geostocker May 10 '17 at 13:47
6
\$\begingroup\$

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");
    
    
            try:
                rover = Rover(x, y, direction, planet);
            exception PlanetError as err:
                print(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:
            return
    

    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:
       break

    rover = Rover(x, y, direction)

    commands = input_commands()

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

    rover.execute(commands)
    planet.add_final_position(rover.get_position())

planet.print_final_positions()

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       
    try:
      width, height = map(int, text.split()[:2])
    except ValueError:
      print("Those were not numbers (nor 'exit'). Try again")
      continue

    # 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")
        self.return_to_start()

And that is rather neat, if I must say so... :-)

\$\endgroup\$
  • \$\begingroup\$ Great review, man! Thank you so much! I've got one question, though. Do you see any way I can refactor the movement in rover.py so that it's easier to extend? I know this is a common coding challenge for jobs and one of the extension problems you might receive later in those challenges is to refactor the movements in a way that doesn't use an if-case or handles more directions than just NESW. Here are two links to blogs that mention such extensions: prabakaranz.blogspot.co.uk/2010/11/day-at-thought-works.html and zerothabhishek.github.io/interview-polymorphism.html \$\endgroup\$ – geostocker May 10 '17 at 19:44
  • \$\begingroup\$ @geostocker, the most common way to simplify that is to have a dictionary where the key is the movement direction, and the value is a tuple with delta movements for both x and y. Understandable? \$\endgroup\$ – holroy May 10 '17 at 19:50
  • \$\begingroup\$ So you basically implement a dictionary that holds the x and y value of a movement in a direction. So whenever a forward movement is being executed you simply check the current direction you are facing and use its corresponding value in the dictionary to add / subtract from the position? DIRECTIONS = {'N': (0, 1), 'E': (1, 0), 'S': (0, -1), 'W': (-1, 0)} DIRECTIONS.get(self._direction) \$\endgroup\$ – geostocker May 10 '17 at 20:25
  • \$\begingroup\$ Tbh that must be wrong... I can't seem to find a good way of grabbing the x and y data out from that tuple in a 'nice' way. Only way I can think of is simply adding the value of each of the elements in the tuple with its corresponding x/y value. That probably would solve it, just not very... neatly. \$\endgroup\$ – geostocker May 10 '17 at 20:27
  • 1
    \$\begingroup\$ @geostocker, if that is the case, you might want to reconsider working for them... \$\endgroup\$ – holroy May 11 '17 at 8:51

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.