# Moving a rover and receiving current coordinates

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

• "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.
– MacD
Commented Jul 21, 2014 at 11:16

## Validation

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.

## Output

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.

## Representation

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

MIN_COORD, MAX_COORD = 1, 1000

class Rover(object):
def __init__(self):
"""Places the rover randomly in the coordinate range with a uniform
distribution"""
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
else:
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

• Note that this also removes the "global" variable r_pos. It seems odd to mix % (move) and str.format (__str__`), though! Commented Jul 21, 2014 at 8:24