Mars Rover in Python

Question

I'm currently applying for a job at a well known company in Stockholm, and I need some help with reviewing my initial draft of my solution to the Mars Rover problem. I mainly use JavaScript when I develop, but the company requires me to solve it using Java, C# or Python.

Luckily for me I've got some basic Python skills, but I'd really like to get some feedback on this initial solution. Even though I realise there might be bugs in the code I'm more focused on best practice, any OOP misses I might have performed (I've never used classes over multiple files before) and such.

Obviously if you find any bugs, that would be great too. I've only worked on the problem for about 3 hours and I've got a week or so left on solving it.

However ugly it is, it does solve the basic test cases. So for all intents and purposes it should "work.

With no further ado, here is the code:

main.py (the brainz)

#!/usr/bin/env python3

from Rover import Rover
from Mars import Mars


def validate_mars(x, y):
    """

    """
    if x.isdigit() and y.isdigit():
        mars = Mars(int(x), int(y))
        Rover.Mars = mars
        return True
    else:
        print("Only numerical elements. Try again!")
        return False

def validate_rover(x, y, direction):
    if x.isdigit() and y.isdigit() and direction.isalpha():
        return True
    return False

def validate_operations(op):
    import re
    pattern = re.compile("^[MRL]*$")

    if pattern.match(op):
        return True
    else:
        return False

def move(op, r):
    try:
        for operation in op:
            if operation == "L":
                r.turnLeft()
            elif operation == "R":
                r.turnRight()
            else:
                r.forward()
    except Exception as err:
        op = input("Your rover went outside of Mars surface.\nCurrently the rover's position \
is {} facing {}. Try again!\n>>> ".format(r.getPosition(), r.getDirection())).upper()
        move(op, r)

def main ():

    #
    # A while loop to validate user input.
    # Using a boolean flag to check the integrity of the loop.
    #

    flag = False
    rovers = []

    while (flag == False):
        choice = input("Please enter the size of mars (2 numbers, separated by a space and higher than 0.)\n>>> ").split()
        if len(choice) == 2:
            flag = validate_mars(choice[0], choice[1])
        else:
            print("Incorrect input. Please ensure you enter a string with two numerical elements")
    print("\n--------------------------------------\n")
    flag = False # reset flag to input rovers

    while (flag == False):
        choice = input("Please enter the current rover's initial position.\nRemember to keep inside Mars limits!\n>>> ").split()
        if len(choice) == 3:
            if(validate_rover(choice[0], choice[1], choice[2])):
                rover = Rover(int(choice[0]), int(choice[1]), choice[2])
                choice = input("Please enter the operations you wish for this rover to execute.\n>>> ").upper()
                if validate_operations(choice):
                    move(choice, rover)
                    rovers.append(rover)
                    print("\n--------------------------------------\n")
            else:
                print("You appear to have entered an incorrect sequence. Two numbers followed by a single character (N,E,S,W)\nTry again!")
        if "exit" in choice:
            print("--------------- Output ---------------")
            for rover in rovers:
                print("| {} is on position: {} facing {}".format(rover.getRoverId(), rover.getPosition(), rover.getDirection()))
            print("--------------------------------------")
            exit()


if __name__ == "__main__":
    main()

Mars.py (the "board")

class Mars ():

    def __init__(self, x, y):
        self.x = x
        self.y = y
        print("Mars initiated with size: " + str(Mars.getX(self)) + ", " + str(Mars.getY(self)))

    def getX(self):
        return self.x

    def getY(self):
        return self.y

Rover.py (the controller)

import operator

directions = ["N", "E", "S", "W"]

class Rover():
    rvrCount = 0 #initialize the count of rovers to 0.
    Mars = ""

    def __init__(self, x, y, direction):
        """
        Initialize a rover and increment the count of rovers by one (1).
        """
        self.direction = direction
        self.x = x
        self.y = y
        Rover.rvrCount += 1
        self.id = "Rover " + str(Rover.rvrCount)
        print("Rover{} initiated on {} {} facing {}".format(str(Rover.rvrCount), str(self.x), str(self.y), self.direction))

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

    @direction.setter
    def direction(self, d):
        if d.upper() not in directions: raise Exception("Direction not correct, use 'N, E, S or W'")
        self._direction = d.upper()

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

    @x.setter
    def x(self, x):
        if (x < 0 or x > Rover.Mars.getX()): raise Exception("This rover's x-value is out of bounds. It should be value should be < 0 and > " + str(Mars.getX()))
        self._x = x

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

    @y.setter
    def y(self, y):
        if (y < 0 or y > Rover.Mars.getY()): raise Exception("This rover's y-value is out of bounds. It should be value should be < 0 and > " + str(Mars.getY()))
        self._y = y

    def getDirection(self):
        return self.direction

    def getPosition(self):
        return str(self.x) + " " + str(self.y)

    def getRoverId(self):
        return self.id

    def turnRight(self):
        self.direction = directions[0] if directions.index(self._direction) == 3 else directions[directions.index(self._direction) + 1]

    def turnLeft(self):
        self.direction = directions[3] if directions.index(self._direction) == 0 else directions[directions.index(self._direction) - 1]

    def forward(self):
        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

Answer 1

main.py

from Rover import Rover
from Mars import Mars

According to PEP 8 (the official style guide for Python):

Modules should have short, all-lowercase names. Underscores can be used in the module name if it improves readability. Python packages should also have short, all-lowercase names, although the use of underscores is discouraged.

_{^{(https://www.python.org/dev/peps/pep-0008/#package-and-module-names)}}

"""

"""

Now that doesn't do much good, does it? That's meant to be a documentation string, but it's blank. Either write some documentation, or don't put those quotation marks there. Somebody might think you're writing in invisible ink.

if x.isdigit() and y.isdigit():

Python is smarter than you are with integers. That's no insult; Python is pretty smart. You may think that a string that has something other than a digit is not convertable to an integer, but that is not true (I'm pretty sure). I actually have failed to come up with an example where .isdigit() fails yet int() can come up with something, but I'm pretty sure there is such a case. Besides that, EAFP (Easier to Ask Forgiveness than Permission) is a more commonly preffered method in Python than LBYL (Look Before You Leap) because it tends to take fewer function calls (as is true here) and you're less likely to have a race condition in multi-threaded code. I would do that here:

try:
    mars = Mars(int(x), int(y))
    Rover.Mars = mars
    return True
except ValueError:
    return False

I also took out the print call because after all this is just a validation function. I would leave it up to the calling function whether or not it should do someting about it.

I'm also not liking the Rover.Mars = ... bit. What if you want more than one Mars depending on the rover? In my opinion, that should be an instance attribute, not a class attribute.

if x.isdigit() and y.isdigit() and direction.isalpha():
    return True
return False

You just defined a function to validate x and y. Why not use it? Since I took out that print call, it can be used here just fine without the user knowing anything about it. As for direction, just being alphabetic does nothing for you. It needs to be one of the direction letters. Next, I have a problem with using an if check in the first place. if executes something if a block of code evaluates to something "truthy". Instead of returning True when something is truthy and False when it isn't, why not just return the truthy thing in the first place? Well, okay; some truthy things are not suitable return values, but here they aren't just truthy or falsey; they are True or False:

from Rover import Rover, directions # <- added directions import

...

def validate_rover(x, y, direction):
    return validate_mars(x, y) and direction in directions

def validate_operations(op):
    import re

According to PEP 8:

Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.

_{^{(https://www.python.org/dev/peps/pep-0008/#imports)}}

Put the import up at the top, not in the function. I don't think there's actually any performance difference, but it's cleaner.

while (flag == False):

I have three problems with this line:

1. flag is not at all a descriptive name. Try to give it a name that says why there's a flag here.
2. Python does not use parentheses like JavaScript does. They're just extra here.

3. There's no need to compare to False. The while condition works only with booleans, so there's no need to create another boolean based on this boolean's comparison; just reverse the boolean:

   while not flag:

The same goes for the next loop.

Besides this, I don't really like the looks of that loop. I would do something like this:

while True:
    choice = input("...").split()
    if len(choice) == 2 and validate_mars(*choice):
        break
    print("Incorrect input...")

Come to think of it, I don't like that you need to do a length check here. I would prefer if validate_mars() would take an iterable as its only argument and do the length check itself.

exit()

I would prefer if this were a simple return. That way, if for example another file imported this and called main(), it could still do other things after main() is done. It's a matter of opinion, but I prefer letting the calling functions make as many decisions as possible. Defaults are good, but I like to make a lot of it be configurable.

Mars.py

def __init__(...):
    ...
    print(...)

Printing is something I think should be left to the calling function. If that's what it wants to say when Mars emerges, so be it; but I don't think Mars should be the one to decide.

def getX(...):
...
def getY(...):

To quote PEP 8:

Function names should be lowercase, with words separated by underscores as necessary to improve readability.

mixedCase is allowed only in contexts where that's already the prevailing style (e.g. threading.py), to retain backwards compatibility.

_{^{(https://www.python.org/dev/peps/pep-0008/#function-names)}}

That is also relevant in several of the methods in Rover.py.

Rover.py

directions = [...]

A list is helpful when you need to maintain a specific order and you need to be able to change it later, but you don't need to change anything for this. That's why I prefer a tuple. You can't use the more-efficient set because order is important here, but the immutable tuple works just fine.

def __init__(...):
    ...
    print(...)

This is the same as in Mars.py. I prefer if the calling function does the printing.

Answer 2

If I were reviewing this as an interview submission, my key concerns would be:

1. No documentation. What is the purpose of each class? What are its responsibilities? What do the methods do? What parameters do they take and what do they return?

2. Use of global variables (Rover.rvrCount and Rover.Mars) indicating an incomplete design.

3. No test cases. (Note that the original challenge text says, "We highly recommend using a unit testing framework".)

4. Design assumes that multiple rovers can occupy the same position without colliding.