Skip to main content
deleted 95 characters in body
Source Link
Phrancis
  • 20.4k
  • 6
  • 68
  • 154

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? 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?

EDIT: Removed my earlier question where I was asking about feedback on my test-cases.

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?

EDIT: Removed my earlier question where I was asking about feedback on my test-cases.

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?

Tweeted twitter.com/StackCodeReview/status/862398180805275657
added 73 characters in body
Source Link
geostocker
  • 671
  • 4
  • 10

A month or so ago I found the Mars rover challenge 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.

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.

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.

Source Link
geostocker
  • 671
  • 4
  • 10

Python solution to Mars Rover

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!

EDIT: Removed my earlier question where I was asking about feedback on my test-cases.


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