Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
share|improve this question
    
"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 Jul 21 at 11:16

1 Answer 1

up vote 6 down vote accepted

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
share|improve this answer
    
Note that this also removes the "global" variable r_pos. It seems odd to mix % (move) and str.format (__str__), though! –  jonrsharpe Jul 21 at 8:24

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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