8
\$\begingroup\$

I'm learning Python at the moment. While taking the first steps, I implemented a Tic Tac Toe game and wanted to share it here, to get some feedback and continue learning. I'm looking for any improvements within my code: style, common mistakes, etc.

def create_playing_board(side_length):
     return [[0 for x in xrange(side_length)] for x in xrange(side_length)] 

def print_current_board(board):
     print "\n", " Y |  ", board[0][2], '|', board[1][2], '|', board[2][2]
     print "   |   --+---+--"
     print "   |  ", board[0][1], '|', board[1][1], '|', board[2][1]
     print "   |   --+---+--"
     print "   |  ", board[0][0], '|', board[1][0], '|', board[2][0]
     print "   |  "
     print "   ------------ X"


def set_marker(player, board):
     print "\nplayer",player, " - x/y input between 0 and 2"

     x = y = 3
     while(x not in range(0,3)):
          x = input("x: ")
     while(y not in range(0,3)):    
          y = input("y: ")

     if board[x][y] == 0:
          board[x][y] = player
          return board
     else:
          print '\nfield already used - choose again!'
          set_marker(player,board)
          return board

def check_for_winner(player, board):
     if (board[0][0] == board[1][0] == board[2][0] == player):
          return 1
     elif (board[0][1] == board[1][1] == board[2][1] == player):
          return 1
     elif (board[0][2] == board[1][2] == board[2][2] == player):
          return 1
     elif (board[0][0] == board[0][1] == board[0][2] == player):
          return 1
     elif (board[1][0] == board[1][1] == board[1][2] == player):
          return 1
     elif (board[2][0] == board[2][1] == board[2][2] == player):
          return 1
     elif (board[0][0] == board[1][1] == board[2][2] == player):
          return 1
     elif (board[0][2] == board[1][1] == board[2][0] == player):
          return 1
     else:
          return 0


board = create_playing_board(3)
player = 1
is_win = 0

print "welcome to tic tac toe"

while(is_win == 0):
     print_current_board(board)    
     board = set_marker(player, board)
     is_win = check_for_winner(player, board)
     if is_win == 1:
          print ' \n### player', player,' wins! ###\n'
          print_current_board(board)
          print "\ndo you guys want to play again?"
          is_win = input("0 = yes // 1 = no: ")
          if is_win == 0:
               board = create_playing_board(3)        
     player = (player%2)+1
\$\endgroup\$
8
\$\begingroup\$

Note:

This answer has gone through several updates... Some areas are updated to enhance clarity.


Although it looks really good, I have a few suggestions.

  1. Use print() instead of print if you want this to work with Python 3.x
  2. range(3) is the same as range(0,3)
  3. Don't use is_win to determine if the player wants to play again. Use an appropriate variable name. Try considering something like play_again.
    • I actually suggest using while True: and breaking out of the while loop if the user says they don't want to play again instead. But you can make it work either way.
  4. When you want to test your code, consider using this code below:
if __name__ == '__main__':
    # play with stuff I created
    # ....
  1. For create_playing_board(side_length)
    • I suggest getting rid of side_length because in a simple tic-tac-toe game, it's always 3 x 3.
    • Consider using different variable for the second generator. Like so:
    return [[0 for x in range(side_length)] for y in range(side_length)]    # Either this
    return [[0 for x in range(3)] for y in range(3)]                         # or this.
  1. input() does not cast the user's input into anything except string. So if you want to compare it with an integer, you need to cast it to integer using int(). See the code line below for a quick way to capture and cast into an integer:
int(input("A question to ask"))
  1. When you ask the player if he or she wants to play again and plan on storing their answer into is_win, I don't think that's a really good idea because is_win is already being used for something else. I would consider this instead:
print("\nDo you guys want to play again?")        # Ask if the user would like to play again.
play_again = int(input("0 = yes // 1 = no: "))

Update #1:

I just caught a couple of things...

  1. How do you determine which player is the winner? -- Never mind.... I found how you did it.
  2. The game just keeps on going, even though the board is filled and there is no winner. Try to add something to check for "It's a tie!"

Update #2:

I noticed a few more opportunities for improvement...

  1. I suggest using boolean values instead of integer to determine one of the two options like yes / no.

    • A way to think about this is:

      If I want to check for a condition, I just want a "Yes" or a "No".
      -- To check for this in programming, it's a "True" or a "False"

    • See the code below:

# is_win = 0        # Instead of this...
is_win = False    # Use this.

# .... Somewhere in your code ....
# This is just a snippet...
def check_for_winner(player, board):
    if board[0][0] == board[1][0] == board[2][0] == player:
        # return 1    # Instead of this...
        return True   # Do this.

# .... Somewhere in your code ....
# if is_win == 1:   # Instead of this...
if is_win:          # Use this...
  1. I found way to check for a Tie. I've created a new Gist for this example. Feel free to see how you can implement this check_for_tie() method in your code:
def check_for_tie(board):
    if not any(0 in sublist for sublist in board):
        return True
    return False
  1. If you plan on implementing #2, consider modifying the code you used to check for is_win to also check for whether there is a tie, and to print the result while minimizing unnecessary redundancy.

  2. Think about simplifying the check_for_winner() method. Check this code block below:

def check_for_winner(player, board):
    # Diagonal
    # This is placed up here first because this runs
    # slightly quicker than the one in the for loop.
    if board[0][0] == board[1][1] == board[2][2] == player or \
            board[2][0] == board[1][1] == board[0][2] == player:
        return True

    # Horizontal or Vertical
    for x in range(3):
        if board[x][0] == board[x][1] == board[x][2] == player or \
                board[0][x] == board[1][x] == board[2][x] == player:
            return True

    return False

Update #3:

Thousands of apologies... I've made quite a list of changes, but I'm just trying to help you out and improve my answer based on Conor Mancone's suggestion.

I've updated my Gist and created a new one for comparison.

  1. This Gist -- the same one I posted earlier in this answer has my up-to-date suggestion.
  2. The 'That' Gist has my alternate suggestion (slight differences).

Note:

Even though I suggested a laundry list of things in this answer, they are not all implemented in my Gist links; however, they can be taken into reflection.


I know this is probably close to a novel, but I hope this helps :)

\$\endgroup\$
  • \$\begingroup\$ thanks a lot for this detailed breakdown - ill rework the code with your input and will post it as soon as im finished :) just wanted to let you know that i really appreciate it :) \$\endgroup\$ – hi im vinzent Jul 13 '17 at 16:28
  • 1
    \$\begingroup\$ @hiimvinzent I updated the code for the check_for_winner() method. -- It's at the end of my answer. \$\endgroup\$ – Sometowngeek Jul 13 '17 at 16:40
  • 1
    \$\begingroup\$ Great review. One problem: your suggested check_for_winner function doesn't check the diagonals. Also, I personally try to avoid while True (and the like), as it implicitly relies upon the contents of the loop to keep you from getting stuck in an infinite loop. That disconnect can make it easy to accidentally create an infinite loop down the road. I like to keep it deterministic when I can. In this case, tic-tac-toe has a maximum of 9 moves, so you could do a for turn in range(9). This will also remove the need to check for a tie (it's a tie if you finish all 9 turns). \$\endgroup\$ – Conor Mancone Jul 13 '17 at 17:21
  • 1
    \$\begingroup\$ Now you're thinking like a real developer :). I suggest using str.isdigit() See this Gist on how to implement it. \$\endgroup\$ – Sometowngeek Jul 14 '17 at 15:55
  • 1
    \$\begingroup\$ @hiimvinzent I just thought of a pretty cool idea. This might be a good challenge for you. Turn the tic tac toe game into a class. I think it'll be a fun challenge :) \$\endgroup\$ – Sometowngeek Jul 16 '17 at 17:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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