Moving around in a 2D grid

Question

Please tell me ways I can improve this code to be more efficient.

#Moving Character in 2D Plane
'''
Made by a beginner
'''
arena = [] # This is a map file

def make_board(place): #Generates map terrain
    for i in range(5):
        place.append(["O"] * 5)

def list_to_string(alist): #Makes map easier to read
    for i in alist:
        print(" ".join(i))

make_board(arena) #Calling the function

prev_char_y = 2 # Keeping track of player's last movement
prev_char_x = 2 #Keeping track of player's last movement
char_y = 2 #Keeping track of player's current movements
char_x = 2 #Keeping track of player's current movements
arena[char_x][char_y] = "X" #What is used to show player pos.
list_to_string(arena) #Shows map with player
go = True # Loop :)
while True:
    try:
        while go:
            print("You are: \nX") #Remind player where he or she is (\n means new line)
            control = input("Move left, right, up, down, or stop?") #Ask what player wants to do
            control = control.lower() #Converts player input to lowercase
            if control == "left": #If left
                char_x = char_x - 1
            elif control == "right":# If right
                char_x = char_x + 1
            elif control == "up": #If up
                char_y = char_y - 1
            elif control == "down": #If down
                char_y = char_y + 1
            elif control == "stop": #If player wants to stop
                go = False
            elif control is not "left" or control is not "right" or control is not "down" or control is not "up":
                print("Please enter a proper direction") #If it's not one of those commands
            arena[prev_char_y][prev_char_x] = "O" #Removes previous player position marker
            arena[char_y][char_x] = "X" #Adds current player position marker (Y, X) instead of (X, Y)
            prev_char_y = char_y #Sets the previous y to the current y
            prev_char_x = char_x #Sets the previous x to current x
            list_to_string(arena) #Prints map
    except IndexError:
        print("That's out of range, please pick somewhere else to move")
        char_y = prev_char_y
        char_x = prev_char_x
        arena[char_y][char_x] = "X"
        list_to_string(arena)

Answer 1

First and foremost, remember DRY: Don't Repeat Yourself. Secondly, group your related functions (preferably in classes; see #1 below). Thirdly, keep your variable names simple and easy to read.

Here are a few more points and examples, elaborating on the above:

1. You can create a class for your Board. This can easily be done by subclassing the builtin list type, because your original arena is just a normal list.

   class Board(list):
   
       def __str__(self):
           return "\n".join(" ".join(row) for row in self)
   

   This class has one method, __str__, which modifies the superclass' method. It returns the board in the format you want; space-delimited cells and linefeed-delimited rows.

   Now, to create our default board, we'll use a list comprehension:

   [["O"] * 5 for _ in range(5)]
   

2. We can get rid of repeated literals such as your various "X", "O", and 2s by creating constant variables:

   MARKER_X = "X"
   MARKER_O = "O"
   START = [2, 2]
   

   We can also put the previous and current player-coordinates into lists. Note that [:] takes the contents of a list; in this case, that list is START.

   curr_pos = START[:]
   prev_pos = START[:]
   

3. Group your functions and other code chunks under a class; I'll call mine Game. I'll split this point into several smaller pieces.

   • Let's subclass the object builtin. Note that, in Python 3, we can omit (object) because it's implicit.

     class Game(object):
     

   • Now let's move in those constant variables we created earlier. These are the class variables that we can access via the Game object (i.e. Game.MARKER_X).

     class Game(object):
     
         MARKER_X = "X"
         MARKER_O = "O"
         CTRLS = [
             "left",
             None,
             "right",
             "up",
             None,
             "down",
         ]
         EXIT = "stop"
         START = [2, 2]
         DEFAULT = [["O"] * 5 for _ in range(5)]
     

     I'll explain the CTRLS and EXIT variables farther down.

   • We need to initialize the game, so we'll write an __init__ method.

     class Game(object):
     
         ... # Existing code
     
         def __init__(self):
             self.flag = True
             self.arena = Board(Game.DEFAULT)
             self.prev_pos = Game.START[:]
             self.curr_pos = Game.START[:]
             self.move_player()
     

     Now we have:

     • created a variable, flag which we'll use as an exit flag (the same as your go variable).

     • arena: a Board created from our defaults.

     • moved our curr_pos and prev_pos variables (from #2) inside our class. These are instance variables that we can access via self.

     • setup the player's initial position (explained in detail, below).

   • Let's handle our player's movements. I'm going to call this move_player, which I mentioned in the previous point. We're going to use the instance's curr_pos and prev_pos variables for this. See the code comments for detailed explanations.

     class Game(object):
     
         ... # Existing code
     
         def move_player(self):
             px, py = self.prev_pos                        # Split apart the previous x and y coordinates.
             cx, cy = self.curr_pos                        # Split apart the current x and y coordinates.
             if (-1 < cx < 5) and (-1 < cy < 5):           # Check to make sure our coordinates are valid.
                 self.arena[py][px] = Game.MARKER_O        # Set the previous position to empty.
                 self.arena[cy][cx] = Game.MARKER_X        # Set our current position to our marker.
             else:                                         # If the coordinates are invalid, don't do anything.
                 print("Please enter a proper direction.") # Notify the player.
                 self.curr_pos = self.prev_pos[:]          # The "new" position becomes the old position.
                 self.move_player()                        # Move the player back.
     

   • Finally let's add in our user interface. I'm going to call this the play method. I've added code comments for a detailed explanation.

     class Game(object):
     
         ... # Existing code
     
         def play(self):
             print("You are: \nX")
             while self.flag:
                 print(str(self.arena))
                 ctrl = input("Move left, right, up, down, or stop?").lower()
                 if ctrl in Game.CTRLS:                              # Check to see if the command is valid.
                     d = Game.CTRLS.index(ctrl)                      # Find our command's index in the command-list. This is important.
                     self.prev_pos = self.curr_pos[:]                # The current position becomes the previous position.
     
                     self.curr_pos[d > 2] += d - (1 if d < 3 else 4) # Here, we take the command's index in the command-list, and determine:
                                                                     #   1: whether we're moving horizontally or vertically.
                                                                     #   2: which direction we're moving (i.e. left or right).
                                                                     # Then, we take the command's index and compensate to move either +1 or -1.
                                                                     # For example, if the command is "up", we take the index, 3, and subtract 4 to get -1.
     
                     self.move_player()                              # Try moving the player to his new postion.
                 elif ctrl == Game.EXIT:                             # Check to see if the command is to exit.
                     self.flag = False                               # Set the flag to False. This will end the game.
                 else:                                               # Check if the player used an invalid command.
                     print("Please enter a proper direction.")       # Notify the player.
     

   • Now we can play the game.

     my_game = Game() # Create a game.
     my_game.play()   # Start the game.
     

---

Here is our final code:

class Board(list):

    def __str__(self):
        return "\n".join(" ".join(row) for row in self)


class Game(object):

    MARKER_X = "X"
    MARKER_O = "O"
    CTRLS = [
        "left",
        None,
        "right",
        "up",
        None,
        "down",
    ]
    EXIT = "stop"
    START = [2, 2]
    DEFAULT = [["O"] * 5 for _ in range(5)]

    def __init__(self):
        self.flag = True
        self.arena = Board(Game.DEFAULT)
        self.curr_pos = Game.START[:]
        self.prev_pos = Game.START[:]
        self.move_player()

    def move_player(self):
        px, py = self.prev_pos
        cx, cy = self.curr_pos
        if (-1 < cx < 5) and (-1 < cy < 5):
            self.arena[py][px] = Game.MARKER_O
            self.arena[cy][cx] = Game.MARKER_X
        else:
            print("Please enter a proper direction.")
            self.curr_pos = self.prev_pos[:]
            self.move_player()

    def play(self):
        print("You are: \nX")
        while self.flag:
            print(str(self.arena))
            ctrl = input("Move left, right, up, down, or stop?").lower()
            if ctrl in Game.CTRLS:
                d = Game.CTRLS.index(ctrl)
                self.prev_pos = self.curr_pos[:]
                self.curr_pos[d > 2] += d - (1 if d < 3 else 4)
                self.move_player()
            elif ctrl == Game.EXIT:
                self.flag = False
            else:
                print("Please enter a proper direction.")


my_game = Game()
my_game.play()

Answer 2

Your definitions aren't in the ideal order. This is how definitions usually go:

1. Imports
2. Constants
3. Classes and functions
4. Module level code

Remember, arena doesn't need to be defined before make_board() is.

def make_board(place):
    for i in range(5):
        place.append(["O"] * 5)

Do you really want to append those lists to place? What if we already played and are making a new board? You'd have a board that is double-sized. A list comprehension would be better¹:

def make_board(place):
    place[:] = [["O"] * 5 for _ in range(5)]

That replaces everying in place with a bunch of rows of O's. As I mention below, you shouldn't be using magic numbers.

prev_char_y = 2
prev_char_x = 2
char_y = 2
char_x = 2

These are called magic numbers. Why 2? You should use prev_char_y = len(arena) // 2 and prev_char_x = len(arena[prev_char_y]) // 2. You can then use char_y = prev_char_y and char_x = prev_char_x.

area[char_x][char_y] = "X"

You use X, O ... what are they? You are putting them directly in your code without defining them programmatically. They are more magic numbers. Well, they aren't actually numbers, but the concept is the same. Instead, define constants at the beginning of the file for the different characters.

while True:
    try:
        while go:
            ...
    except IndexError:
        ...

You have a bug there. If the user types stop, the program hangs. Why? Because you didn't break out of the outer loop. You need to switch your try-except and while go blocks:

while go:
    try:
        ...
    except IndexError:
        ...

Since the try-except blocks are within while go:, your while True: loop is no longer necessary. Actually, I wouldn't even use try-except here. Instead of picking up an IndexError, add char_y %= len(arena) char_x %= len(arena[char_y]) right after all of the if's and elif's. That way, the user typing right at the right edge brings him back to the left edge instead of giving an error. Your code had an interesting bug anyway. You could go left and wrap around to the right, but you could do it only once. If you keep going left until you reach the left edge again, this time it won't let you. That's because the first time raised no errors. You can easily go to arena[...][-1] because it means to go to the last item in arena[...]. You can't however, go to arena[...][-4].

elif control is not "left" or control is not "right" or ...

It should be equality, not identity-checking. That means control != "left", etc. It should also be using and. If control is down, it is not left. If you use or, that suite would be executed ... except that you already took care of it in your if control == "left": block so this condition isn't checked. In fact, this condition is never checked unless control wasn't taken care of above (isn't any of left, right, up, etc.). That means that it can be a simple else:

---

¹ See also slice assignment