Create two vehicles, move them on a grid based on user input

Question

I have a grid and a class Vehicle, which will have starting point(X, Y on the grid) and direction(one of N,E,S,W) taken from user and there will be commands, L & R turns the vehicle 90 degrees around left and right respectively and M moves the vehicle one unit to faced direction.

For instance, if the command is M and the current direction is E, the vehicle moves from (x,y) to (x+1,y) and preserve its direction.
If the command is R, and the current direction is E, vehicle preserves its position but direction changes to S.

My input consists of 5 lines:

• The first line defines the limits of the grid; X and Y separated by space
• The second line defines the current position and facing direction for the first vehicle; X, Y and Dir are separated by space
• The third line defines the commands for the first vehicle, which is a line of string
• The fourth and fifth lines are the same as second and third except they are for the second vehicle

Note: Vehicles are sent sequentially. If the second vehicle attempts to move to the occupied spot of the first vehicle, the command will be skipped. If any move command makes any of the vehicles move out of the grid, that command will be skipped as well. Inputs are always in expected style, so there is no need to validate inputs.

---

As an example:

Inputs:

6 6 
1 3 E 
RMLLMRMRM 
1 1 N 
LMLML 

Output:

2 2 S 
0 0 E

---

My code:

directions = ['N','E','S','W'] 
movement = {'N': (0,1), 'E': (1,0), 'S': (0,-1), 'W':(-1,0)}
commands = {'L': 'turn_left', 'R': 'turn_right', 'M': 'move'}
GRID_MAX_X, GRID_MAX_Y = map(int, raw_input().split())
first_vehicle_x = None
first_vehicle_y = None
class Vehicle():
 def __init__(self, x, y, face):
 self.x = x
 self.y = y
 self.dir = face
 def turn_left(self):
 self.dir = directions[(directions.index(self.dir)-1)%len(directions)]
 def turn_right(self):
 self.dir = directions[(directions.index(self.dir)+1)%len(directions)]
 def move(self):
 new_x = self.x + movement[self.dir][0]
 new_y = self.y + movement[self.dir][1]
 if new_x != first_vehicle_x or new_y != first_vehicle_y:
 if new_x in xrange(GRID_MAX_X+1):
 self.x = new_x
 if new_y in xrange(GRID_MAX_Y+1):
 self.y = new_y
vehicle_one_pos = raw_input().split()
vehicle_one_commands = raw_input()
vehicle_one = Vehicle(int(vehicle_one_pos[0]), int(vehicle_one_pos[1]), vehicle_one_pos[2])
for command in vehicle_one_commands:
 eval("vehicle_one.{0}()".format(commands[command]))
first_vehicle_x = vehicle_one.x
first_vehicle_y = vehicle_one.y
vehicle_two_pos = raw_input().split()
vehicle_two_commands = raw_input()
vehicle_two = Vehicle(int(vehicle_two_pos[0]), int(vehicle_two_pos[1]), vehicle_two_pos[2])
for command in vehicle_two_commands:
 eval("vehicle_two.{0}()".format(commands[command]))
print vehicle_one.x, vehicle_one.y, vehicle_one.dir
print vehicle_two.x, vehicle_two.y, vehicle_two.dir

About my code, it feels like repetition on doing same things for each vehicle but wasnt't sure how to handle without it since I need position of vehicle_one for vehicle_two.

Any comment is always appreciated, thanks in advance.

• Tolani: Could you improve your title to capture what your code is about
• Brandon Bishop: @Tolani Well it's about creating and moving two vehicles. Tried again. Of course, feel free to edit it.

Answer 1

Use functions

Having code at the top-level of a module is bad practice as it impairs both reusability and testing: as soon as the module is imported, the code gets executed which is not necessarily what you want.

Instead you should use functions and wrap the remaining top-level code in an if __name__ == '__main__': statement.

Having functions would also mean that you need to stop relying on global variables such as GRID_MAX_X, GRID_MAX_Y, first_vehicle_x or first_vehicle_y. Which is a good thing as currently they make your code tightly coupled. Instead, you should pass those values of interest as parameters to objects or functions.

Bug

Requirements state that:

Vehicles are sent sequentially. If the second vehicle attempts to move to the occupied spot of the first vehicle, the command will be skipped. If any move command makes any of the vehicles move out of the grid, that command will be skipped as well.

You effectively skip the command if the second vehicule tries to move to the position of the first one, but you don't skip it if the move would put the vehicule out of the boundaries of the map. Instead you make the vehicule glide along that boundary.

Also note that you can easily compare the position of the two vehicules using tuple equality (new_x, new_y) != vehicule_one_position and check that a number is within bounds using extended comparisons: 0 <= new_x <= grid_max_x.

Indexables

Strings are iterables and indexables, no need to split them in a list or a tuple. I would however encapsulate changing directions in its own class to better separate concerns and avoid a small amount of repetitions.

eval

There is one saying that eval is evil. It has its uses, but never in such simple cases. Instead, you should know that you can reference functions and methods just by their name so you can build the commands dictionnary in the Vehicule class using

{
 'L': self.turn_left,
 'R': self.turn_right,
 'M': self.move,
}

And be able to do commands[command]() directly, without the need for eval.

Use @property

For simpler data extraction, @property methods can help format them and appear as a simple attribute access. This help make a cleaner design.

Proposed improvements

class Directions:
 """Circular buffer of possible directions"""
 DIRECTIONS = 'NESW'
 def __init__(self, start):
 self.index = self.DIRECTIONS.index(start)
 def next(self):
 self.index = (self.index + 1) % len(self.DIRECTIONS)
 def previous(self):
 self.index = (self.index - 1) % len(self.DIRECTIONS)
 @property
 def current(self):
 return self.DIRECTIONS[self.index]
class Vehicle():
 MOVEMENT = {'N': (0, 1), 'E': (1, 0), 'S': (0, -1), 'W':(-1, 0)}
 def __init__(self, x, y, facing, grid, obstacle):
 self.x = x
 self.y = y
 self.facing = Directions(facing)
 self.grid_width, self.grid_height = grid
 self.obstacle = obstacle
 @property
 def direction(self):
 return self.facing.current
 @property
 def position(self):
 return (self.x, self.y)
 def parse_commands(self, commands):
 action = {
 'L': self.facing.previous,
 'R': self.facing.next,
 'M': self.move,
 }
 for command in commands:
 action[command]()
 def move(self):
 offset_x, offset_y = self.MOVEMENT[self.facing.current]
 x = self.x + offset_x
 y = self.y + offset_y
 if (x, y) != self.obstacle and 0 <= x <= self.grid_width and 0 <= y <= self.grid_height:
 self.x = x
 self.y = y
def setup_and_move_vehicule(grid, obstacle):
 x, y, facing = raw_input().split()
 vehicule = Vehicule(int(x), int(y), facing, grid, obstacle)
 vehicule.parse_commands(raw_input().strip())
 return vehicule.position, vehicule.direction
def main():
 grid = map(int, raw_input().split())
 v1_pos, v1_dir = setup_and_move_vehicule(grid, None)
 v2_pos, v2_dir = setup_and_move_vehicule(grid, v1_pos)
 print v1_pos[0], v1_pos[1], v1_dir
 print v2_pos[0], v2_pos[1], v2_dir
if __name__ == '__main__':
 main()

• Brandon Bishop: I know eval is evil in general but I don't see why it's in this case. I am using it on only handwritten values in code not on user-inputs. If user enters anything but RML, program won't reach eval anyway because of KeyError.
• Brandon Bishop: Other than that, thanks for your answer. This is the first time I see decorators at work so it will help a lot for me to get in that concept.
• 301_Moved_Permanently: @BrandonBishop Yes, in this case eval is rather safe as you know what you're doing. But better safe than sorry, learn to avoid it as hard as you can so you can grasp its value when you no longer can.

Answer 2

Here are some of the potential improvements:

• don't use eval() - it is not safe and is error prone. Instead, use getattr() to get an instance method by it's name:

  getattr(vehicle_one, commands[command])()
  

• don't use globals, pass GRID_MAX_X, GRID_MAX_Y, first_vehicle_x and first_vehicle_y as arguments to the Vehicle instance methods. For example, the maximum x and y values should probably be passed to Vehicle during the initialization

• there is an extra overhead in checking the new x or y values to be within the grid. Instead of the "in xrange" check, simply use the comparison operators:

  if new_x <= GRID_MAX_X:
  

• it feels like having a separate Grid class might lead to a cleaner solution, just a thought

• you can unpack the x, y and direction values to improve on readability:

  x, y, face = raw_input().split()
  vehicle_one = Vehicle(int(x), int(y), face)
  

• according to PEP8, you need to have spaces around operators; for instance, replace:

  self.dir = directions[(directions.index(self.dir)-1)%len(directions)]
  

  with:

  self.dir = directions[(directions.index(self.dir) - 1) % len(directions)]
  

• the parenthesis after the Vehicle() are redundant, you can remove them

• put the main program logic into the if __name__ == '__main__': to avoid it being executed while importing

• use print() function for Python 3.x compatibility

• if the Python 3.x compatibility is something you want to improve on, use input() method and this cross-python compatible snippet

---

There is also some code repetition in reading the vehicle position and commands. The code would only work for two vehicles and does not scale.

You should initialize the loop over the number of possible vehicles, keeping track of the positions currently occupied by previously processed vehicles. Something along these lines (some other mentioned improvements are also applied):

VEHICLE_COUNT = 2
DIRECTIONS = ['N', 'E', 'S', 'W']
MOVEMENT = {'N': (0, 1), 'E': (1, 0), 'S': (0, -1), 'W': (-1, 0)}
COMMANDS = {'L': 'turn_left', 'R': 'turn_right', 'M': 'move'}
class Vehicle:
 def __init__(self, x, y, max_x, max_y, face, occupied_cells):
 self.x = x
 self.y = y
 self.max_x = max_x
 self.max_y = max_y
 self.face = face
 self.occupied_cells = set(occupied_cells)
 def turn_left(self):
 self.face = DIRECTIONS[(DIRECTIONS.index(self.face) - 1) % len(DIRECTIONS)]
 def turn_right(self):
 self.face = DIRECTIONS[(DIRECTIONS.index(self.face) + 1) % len(DIRECTIONS)]
 def move(self):
 new_x = self.x + MOVEMENT[self.face][0]
 new_y = self.y + MOVEMENT[self.face][1]
 if (new_x, new_y) not in self.occupied_cells:
 if new_x <= self.max_x:
 self.x = new_x
 if new_y <= self.max_y:
 self.y = new_y
if __name__ == '__main__':
 max_x, max_y = map(int, raw_input().split())
 occupied_cells = set([])
 results = []
 for _ in range(VEHICLE_COUNT):
 x, y, face = raw_input().split()
 vehicle = Vehicle(int(x), int(y), max_x, max_y, face, occupied_cells)
 for command in raw_input():
 getattr(vehicle, COMMANDS[command])()
 occupied_cells.add((vehicle.x, vehicle.y))
 results.append((vehicle.x, vehicle.y, vehicle.face))
 for result in results:
 print(' '.join(result))

I still don't particularly like the way we pass maximum x and y values and occupied cells to the Vehicle constructor. There is some "separation of concerns" problem here, a vehicle should not know about the dimensions of a grid and other occupied cells - there should be a new "grid" class introduced that would decide if a vehicle is allowed to move to a specified cell. Anyways, hope this is a good starting point for your refactoring.

• Brandon Bishop: Creating another Grid class seems plausible. Will try that. Thanks for your answer and detailed explanations. Sorry, I can only mark one answer as accepted.