Return to Answer

• 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

• 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

Here are some of the areas you may improve the code:

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

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(2):
 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))

Here are some of the potential improvements:

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.

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.

• 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, direction = raw_input().split().strip() 
   vehicle_one = Vehicle(int(x), int(y), direction)
  

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

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

• 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)]
  

There is also some code repetition in reading the vehicle position and commands. The code would only work for two cars 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):

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(2):
 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))