Beginner Tic Tac Toe game in Python

Question

I've just started learning python and I tried making a tic tac toe game and would really appreciate it if I got some feedback. I know, my code isn't the best optimized but this is what I could come up with.

row1 = ['/','/','/']
row2 = ['/','/','/']
row3 = ['/','/','/']

def xsturn():
    row = input('X, which row?')
    place = input('X, which place?') - 1
    if row == 1:
        if row1[place] == '/':
            row1[place] = 'X'
        else:
            print 'Move not possible!'
            xsturn()
    elif row == 2:
        if row2[place] == '/':
            row2[place] = 'X'
        else:
            print 'Move not possible!'
            xsturn()
    elif row == 3:
        if row3[place] == '/':
            row3[place] = 'X'
        else:
            print 'Move not possible!'
            xsturn()

def osturn():
    row = input('O, which row?')
    place = input('O, which place?') - 1
    if row == 1:
        if row1[place] == '/':
            row1[place] = 'O'
        else:
            print 'Move not possible!'
            osturn()
    elif row == 2:
        if row2[place] == '/':
            row2[place] = 'O'
        else:
            print 'Move not possible!'
            osturn()
    elif row == 3:
        if row3[place] == '/':
            row3[place] = 'O'
        else:
            print 'Move not possible!'
            osturn()

def win():
    value = 0
    if row1[0] == row1[1] == row1[2]:
        if row1[0] != '/':
            value += 1
    elif row2[0] == row2[1] == row2[2]:
        if row2[0] != '/':
            value += 1
    elif row3[0] == row3[1] == row3[2]:
        if row3[0] != '/':
            value += 1
    elif row1[0] == row2[0] == row3[0]:
        if row1[0] != '/':
            value += 1
    elif row1[1] == row2[1] == row3[1]:
        if row1[1] != '/':
            value += 1
    elif row1[2] == row2[2] == row3[2]:
        if row1[2] != '/':
            value += 1
    elif row1[0] == row2[1] == row3[2]:
        if row1[0] != '/':
            value += 1
    elif row1[2] == row2[1] == row3[0]:
        if row1[2] != '/':
            value += 1
    return value

def printingrows():
    print row1
    print row2
    print row3

def game():
    while win() == 0:
        xsturn()
        printingrows()
        if win() == 1:
            print 'X won the game!'
            break
        else:
            osturn()
            printingrows()
            if win() == 1:
                print'O won the game!'
                break
game()

Answer 1

I ran your code, and played a bit:

$ python2 test.py
X, which row?1
X, which place?1
['X', '/', '/']
['/', '/', '/']
['/', '/', '/']
O, which row?2
O, which place?2
['X', '/', '/']
['/', 'O', '/']
['/', '/', '/']
X, which row?3
X, which place?1
['X', '/', '/']
['/', 'O', '/']
['X', '/', '/']
O, which row?2
O, which place?1
['X', '/', '/']
['O', 'O', '/']
['X', '/', '/']
X, which row?2
X, which place?4
Traceback (most recent call last):
  File "test.py", line 95, in <module>
    game()
  File "test.py", line 84, in game
    xsturn()
  File "test.py", line 15, in xsturn
    if row2[place] == '/':
IndexError: list index out of range

First, there is no presentation of the board when play starts. So you're relying on the X player to know what's going on.

Second, using two lines to ask for the move raises the issue of making a mistake and needing to go back. I'd suggest you either add a "go back" feature of some kind, or convert to using a single line of input for the entire move- some kind of X, Y input, or numbering the cells from 1-9.

Third, you don't do any range checking on the input, as is obvious from the end of my game. Given that you don't prompt the user with a definition of what input is valid (1-3), that's a double error. You need to tell the user what's valid, and double-check the input to make sure that it is, in fact, valid.

Fourth, you need to improve the presentation of the board. You should display it prior to each move, even the first move, so the players know what's what. You should also label the board, so that input is obvious. If you ask what row, column for a move, then label the rows/columns on the board display. And please change the '/' into something more sensible - use a space (' ') or a dot ('.') or something that looks like an empty cell.

That said, let's look at some code:

def xsturn():
    row = input('X, which row?')
    place = input('X, which place?') - 1
    if row == 1:
        if row1[place] == '/':
            row1[place] = 'X'
        else:
            print 'Move not possible!'
            xsturn()
    elif row == 2:
        if row2[place] == '/':
            row2[place] = 'X'
        else:
            print 'Move not possible!'
            xsturn()
    elif row == 3:
        if row3[place] == '/':
            row3[place] = 'X'
        else:
            print 'Move not possible!'
            xsturn()

def osturn():
    row = input('O, which row?')
    place = input('O, which place?') - 1
    if row == 1:
        if row1[place] == '/':
            row1[place] = 'O'
        else:
            print 'Move not possible!'
            osturn()
    elif row == 2:
        if row2[place] == '/':
            row2[place] = 'O'
        else:
            print 'Move not possible!'
            osturn()
    elif row == 3:
        if row3[place] == '/':
            row3[place] = 'O'
        else:
            print 'Move not possible!'
            osturn()

These two functions are all but identical. Replace them with a single function taking a character parameter:

def take_turn(sigil):
    row = input(sigil + ', which row?')
    place = input(sigil + ', which place?') - 1
    if row == 1:
        if row1[place] == '/':
            row1[place] = sigil
        else:
            print 'Move not possible!'
            take_turn(sigil)
    # etc.

You can then modify the game function to pass in the player sigil:

def game():
    while win() == 0:
        take_turn('X')
        printingrows()
        if win() == 1:
            print 'X won the game!'
            break
        else:
            take_turn('O')
            printingrows()
            if win() == 1:
                print'O won the game!'
                break

Next, rename printingrows to something more accurate: show_board or some such. Try to always write your code using the vocabulary of the problem domain. In this case, the problem domain is a board game.

Finally, there are a bunch of opportunities for you to tighten things up. For example, checking for a win in the turn function instead of separately, or using a list-of-lists instead of three separate row lists. If you can make the changes above and update your code, then we can get into those changes.

Answer 2

After I play the game once, and I call game() again without needing to reset row1, row2, and row3. It doesn't do anything because row1, row2, and row3 are still populated from the earlier game, telling it that the game is over. It might be good to reset those after the game has completed.

Also, if I throw in some weird values, it is not able to handle it gracefully. i.e.

    X, which place?e
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in game
  File "<stdin>", line 3, in xsturn
  File "<string>", line 1, in <module>
NameError: name 'e' is not defined

Answer 3

There are already some good answers here, but I want to provide another method for reducing some of the duplication in your code, specifically the fact that you have 1 method per player turn. osturn() and xsturn(). As Austin pointed out, these can be turned into just one method and you pass in the player who's turn it is. But you can also remove some of your if elif statements to make it even simpler.

your current board is 3 separate rows, row1, row2 and row3. If we make this a dict, we can cut out some duplication.

row1 = ['/', '/', '/']
row2 = ['/', '/', '/']
row3 = ['/', '/', '/']

grid = {
    0: row1,
    1: row2,
    2: row3
}

now we can access rows via grid[0], grid[1] and grid[2]

now consider this turn(symbol) function.

def turn(symbol):
    assert symbol in ("X", "O")
    row = int(input("{}, which row?".format(symbol))) - 1  # -1 here to for the grid keys
    place = int(input("{}, which place?".format(symbol))) - 1
    grid_row = grid[row]
    grid_row[place] = symbol if grid_row[place] == "/" else "/"
    valid_choice = grid_row[place] == "/"
    if valid_choice:
        turn(symbol)

so we don't need to do any if-else checks here. Now I haven't added in any checks to make sure the user enters a valid value. This is a problem that was pointed out in the other answers.

I would like to also add that I don't think recursion is necessary and only adds complexity that we don't need. A simple loop to keep prompting the user to enter a valid turn would be better I think.

I don't like win() == 0 and win() == 1 0 and 1 are magic numbers here and should be avoided.

this would be a lot more readable if we have a meaningful names instead.

if game_has_winner():
    if x_won_game():
        print 'X won the game!'
        break

    print'O won the game!'
    break

now there can be no confusion with who won the game.

I also don't like win itself as a function name, it doesn't really say what it's doing. I would prefer check_for_winner, has_winner or check_victory_condition or something more along those lines. Don't worry about trying to keep names short, make them as clear as possible.