I made a little text-based game where you move a rover and it gives you the current coordinates. You can essentially move around a 1000x1000 grid, and get output into stdout on what your coordinates are. I'm simply wondering if I could improve anything.

# Rover moving program
from random import randint 

# Starting position for rover
r_pos = {'x': randint(1, 1000), 'y': randint(1, 1000)}

def move_rover(x_pos, y_pos):
    if x_pos <= 1000 and y_pos <= 1000:
        r_pos['x'] = x_pos
        r_pos['y'] = y_pos
        print('x{} y{}'.format(
            r_pos['x'], r_pos['y']))
    elif x_pos > 1000 and y_pos > 1000:
        print('Invalid position: x{} y{}'.format(
            x_pos, y_pos))

You run it like this.

>>> import rover
>>> rover.move_rover(234, 789)
x234 y789
  • 1
    \$\begingroup\$ "I made a little text-based game where you move a rover and it gives you the current coordinates." I doubt that; this is a standard test question called 'the Mars rover'. It is quite well-known. \$\endgroup\$
    – MacD
    Commented Jul 21, 2014 at 11:16

1 Answer 1



Your validation is weird, and probably not what you intended.

  • move_rover(-1, -1) appears to succeed, even though it's not within your 1000 × 1000 grid.
  • move_rover(-1, 1001) is a no-op. move_rover(1001, 500) is a no-op.

move_rover(2000, 2000) prints a message. However, it would be more idiomatic to raise some kind of ValueError. Printing the error message directly limits the reusability of your code.


Similarly, I advise against having your move_rover() function also print the new coordinates. Each function should do one thing only. Printing should be a separate operation.

There is repetition in the code to format the position in the success and error cases. The formatting code should be factored out to a common routine.


Using a dictionary with keys named 'x' and 'y' seems cumbersome. You could use a tuple, a namedtuple` or class. A class probably make sense, since your rover should act as an object that responds to messages.

Suggested implementation

from random import randint


class Rover(object):
    def __init__(self):
        """Places the rover randomly in the coordinate range with a uniform
        self.x = randint(MIN_COORD, MAX_COORD)
        self.y = randint(MIN_COORD, MAX_COORD)

    def move(self, x, y):
        if MIN_COORD <= x <= MAX_COORD and MIN_COORD <= y <= MAX_COORD:
            self.x, self.y = x, y
            raise ValueError('Invalid position: %s' % (self))

    def __str__(self):
        """Reports the position of the rover"""
        return 'x{} y{}'.format(self.x, self.y)

You run it like this:

>>> from rover import Rover
>>> rover = Rover()
>>> print(rover)
x944 y556
>>> rover.move(234, 789)
>>> print(rover)
x234 y789
  • \$\begingroup\$ Note that this also removes the "global" variable r_pos. It seems odd to mix % (move) and str.format (__str__), though! \$\endgroup\$
    – jonrsharpe
    Commented Jul 21, 2014 at 8:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.