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've just got a Raspberry Pi, so I thought I'd try my hand at some Python. This is my first ever Python program (hurrah!).

It's a very simple command-line implementation of noughts and crosses. The computer opponent's strategy is simply to pick at random, the code is very verbose in places and there's no way to restart the game without closing and restarting the program, but my aim wasn't to make a working game; I just wanted to practice using the language.

Really, I'm just looking for help making this more Python-y, and any features of the language I've missed that would have been more effective. I've come from several years of C# and Java, so the language feels a little jarring to me right now.

P.S. this is Python 3.2

import random
import sys

#### Initialising ####

boxes = [[0, 0, 0], # 0 - empty
         [0, 0, 0], # 1 - nought
         [0, 0, 0]] # 2 - cross

#### Functions ####

#Converts the box's magic number into a character
def intToText(num):
    if (num == 0): return ' '
    if (num == 1): return 'O'
    if (num == 2): return 'X'

#This outputs the grid, with the correct symbols in the boxes
def printGrid():
    print('┌─┬─┬─┐' '\n'
          '│' + intToText(boxes[0][0]) + '│' + intToText(boxes[1][0]) + '│' + intToText(boxes[2][0]) + '│' '\n'
          '├─┼─┼─┤' '\n'
          '│' + intToText(boxes[0][1]) + '│' + intToText(boxes[1][1]) + '│' + intToText(boxes[2][1]) + '│' '\n'
          '├─┼─┼─┤' '\n'
          '│' + intToText(boxes[0][2]) + '│' + intToText(boxes[1][2]) + '│' + intToText(boxes[2][2]) + '│' '\n'
          '└─┴─┴─┘')

def checkVictory():

    #This method looks at each box in the grid and checks the two boxes
    #in each of the four directions defined as vectors below

    for i in range(0, 3):
        for j in range(0, 3):

            if (boxes[i][j] == 0):
                continue

            for vector in [[1, 0], [1, 1], [0, 1], [-1, 1]]: #The four directions to check for a complete line in

                try:
                    boxToCheck = [i, j]
                    charToCheckFor = boxes[i][j]
                    for x in range(1, 3):

                        boxToCheck[0] += vector[0]
                        boxToCheck[1] += vector[1]

                        #Check if the box contains the same symbol as the previous ones in the line
                        if (boxes[boxToCheck[0]][boxToCheck[1]] != charToCheckFor):
                            break

                        #If we're on the last box in the loop and haven't broken out yet,
                        #we've found 3 in a row. Return the character in the box.
                        if (x == 2):
                            return intToText(boxes[i][j])

                except:
                    continue
    return ' '

def chooseComputerMove():

    #This just fills a list with all the empty boxes and chooses one at random
    emptyBoxes = []
    for i in range(0, 3):
        for j in range(0, 3):
            if (boxes[i][j] == 0):
                emptyBoxes += [[i, j]]

    return emptyBoxes[random.randint(1, len(emptyBoxes) - 1)]

#### Program ####

input('Welcome to noughts and crosses! Press enter to start.')

print('\n' 'You are playing as crosses')
printGrid()

while(1):


    while(1): #Loop until valid input is entered

        move = input('\n' 'Your turn. Make your move:' '\n')

        if (move == 'help'):
            print('Type the coordinates (originating from the top left) of the box you want to put a cross into in the format \'x y\' (e.g. 3 2)')
            print('')
            continue

        if (len(move) == 3):
            if (1 <= int(move[0]) <= 3 and 1 <= int(move[2]) <= 3): #Check the user has entered valid coordinates
                if (boxes[int(move[0]) - 1][int(move[2]) - 1] == 0): #Check that the chosen box is empty
                    boxes[int(move[0]) - 1][int(move[2]) - 1] = 2 #Put an X in the box
                    printGrid()
                    break

        print('Invalid input. Type \'help\' if you\'re stuck')

    # Check if the player's move won the game
    victoryResult = checkVictory()
    if (victoryResult == 'X'):
        print ('You win!')
        break

    # Make the computer's move
    computerMove = chooseComputerMove()
    boxes[computerMove[0]][computerMove[1]] = 1
    print('\n' 'Computer\'s turn:')
    printGrid()

    # Check if the computer's move won the game
    victoryResult = checkVictory()
    if (victoryResult == 'O'):
        print ('Computer wins!')
        break

sys.exit
share|improve this question

1 Answer 1

up vote 16 down vote accepted

1. Introduction

If this is your first Python program, then it's not too bad at all. There are a whole host of things that can be improved, but there are also a good few things you got right. For example, your code is pretty well-commented. Also, I liked the response "Invalid input. Type 'help' if you're stuck" which came at just the right time.

On to the review!

2. Bugs

  1. The computer doesn't play very well:

    ┌─┬─┬─┐
    │ │ │X│
    ├─┼─┼─┤
    │X│O│X│
    ├─┼─┼─┤
    │O│X│O│
    └─┴─┴─┘
    
    Computer's turn:
    ┌─┬─┬─┐
    │ │O│X│
    ├─┼─┼─┤
    │X│O│X│
    ├─┼─┼─┤
    │O│X│O│
    └─┴─┴─┘
    

    It's one thing not to have any strategic look-ahead, but quite another to miss an easy win like this!

  2. There's a bug in the win determination:

    Your turn. Make your move:
    1 1
    ┌─┬─┬─┐
    │X│O│X│
    ├─┼─┼─┤
    │X│O│X│
    ├─┼─┼─┤
    │O│X│O│
    └─┴─┴─┘
    You win!
    

    This happens because your win determination code starts from each of the nine squares and walks for two squares in each of four directions checking to see if the three cells form a line. You rely on getting an IndexError if this walks off the side of the array of boxes. But that's not always going to happen, because Python is perfectly happy for you to supply negative array indices: these count backwards from the end of the array, so that a[-1] is the last element, a[-2] is the second-to-last, and so on.

    One of these searches starts at box (0, 0), and then walks in the direction (−1, 1), getting to (−1, 1) which is the same as (2, 1), and then to (−2, 2) which is the same as (1, 2). All three cells contain an X, so it thinks that you have won!

    So you really do need to check that the indices are in range here.

  3. Here's another bug. The program raises an exception if the game is drawn.

    Your turn. Make your move:
    2 1
    ┌─┬─┬─┐
    │X│O│X│
    ├─┼─┼─┤
    │X│X│O│
    ├─┼─┼─┤
    │O│X│O│
    └─┴─┴─┘
    
    Traceback (most recent call last):
      ...
      File "./cr15631.py", line 78, in chooseComputerMove
        return emptyBoxes[random.randint(1, len(emptyBoxes) - 1)]
      ...
    ValueError: empty range for randrange() (1,0, -1)
    

    You need to check for draws as well as wins.

3. Coding style and technique

  1. Python has a common set of coding conventions, which are written down in "PEP8". The majority of Python developers follow these conventions, so if you learn them too then you will find it easier to collaborate with other Python programmers. Some of these you follow already, but you might take a look at "Function names should be lowercase, with words separated by underscores as necessary to improve readability."

  2. Your code has good descriptions of each function in the form of comments. In Python it's usual to put the description of a function into a docstring which can users can retrive using the built-in help function.

    Often you'll find that the discipline of writing documentation for a user helps you choose a better name for your function. For example, instead of

    #Converts the box's magic number into a character
    def intToText(num):
        if (num == 0): return ' '
        if (num == 1): return 'O'
        if (num == 2): return 'X'
    

    you could write:

    def box_display(num):
        """
        Return the character to display for a box containing `num`.
        """
        return ' OX'[num]
    
  3. Instead of using + to join lots of strings together, you can use Python's powerful string formatting mechanism. Your printing code could be written:

    def print_grid():
        """
        Print the grid of boxes.
        """
        print('┌─┬─┬─┐\n'
              '│{}│{}│{}│\n'
              '├─┼─┼─┤\n'
              '│{}│{}│{}│\n'
              '├─┼─┼─┤\n'
              '│{}│{}│{}│\n'
              '└─┴─┴─┘\n'
              .format(*[box_display(boxes[i][j])
                        for i in range(3)
                        for j in range(3)]))
    

    (I've used a couple of advanced features in the last three lines to avoid the repetition of box_display(boxes[0][0]), box_display(boxes[0][1]), ... and so on: first, a list comprehension to build the list of characters to pass to the format method, and secondly the *args syntax to pass arguments to the function as a list instead of individual parameters.)

  4. range(0, 3) can be abbreviated to range(3).

  5. Python doesn't need parentheses around the test in an if statement.

  6. In the win determination code, you test for the last time around a loop with your test if x == 2:. Python has syntax for handling the last time around a loop with the else: clause to the for statement.

  7. You statement sys.exit doesn't actually call the function: you'd need to write sys.exit() for that. But there's no point in doing so: when a Python program comes to an end it exits automatically without you having to tell it to do so.

4. Design improvements

  1. There's no particular need to store the box contents as numbers (and then to convert them to letters for display). Why not just store O and X?

  2. You'll find that a lot of things become easier if you don't try to keep the board in a 3-by-3 array, but in a 9-element list. That way, you only need one loop over the elements instead of two nested loops. It will be easier for the player to type in a number from 1 to 9 than to try to remember which co-ordinate axis is which.

    And for the win determination, you can just keep a list of all the winning lines, as there are only eight of them:

    winning_lines = [(0, 1, 2), (3, 4, 5), (6, 7, 8), (0, 3, 6),
                     (1, 4, 7), (2, 5, 8), (0, 4, 8), (2, 4, 6)]
    

That's probably enough for you to be going on with. Good luck, and enjoy your Python programming.

share|improve this answer
    
Thanks! That's a lot to digest... I'm sort of ashamed that I didn't think of some of those things myself too. Anyway, again, thanks. –  Tharwen Sep 14 '12 at 23:33
    
+1 On keeping a list of winning lines, often relatively less 'smarter' functions perform superior to the smarter counter parts, especially with small data sets. –  subiet Sep 17 '12 at 5:55
    
That finally explained the *args syntax to me! Thank you! –  Jonas Gröger Jan 21 '13 at 3:37

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.