6
\$\begingroup\$

I'm pretty new to Python - did some Fortran 77 at University for a minor project. Programming skills are a bit rusty!

I plan to add a computer opponent next ...

This is written in Python 2.7.

#   Inspired by: https://codereview.stackexchange.com/questions/15631/command-line-noughts-and-crosses
#   Programmed by: D Innes, Langley School, NR14 6BJ.
#   Created: 12/12/17


#### Variables ####

turn_num = 1
board = [" "," "," "," "," "," "," "," "," "]
ender = False
chosen_move = 0

#### Functions ####

def message_box(comment): # Used to display a message in a common format
    print
    print "- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -"
    print "-    %s" % comment
    print "- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -"
    print    

def display_board(board): # Displays the current state of the board
    print '     %s | %s | %s' % (board[0],board[1],board[2])
    print '    ---+---+---'
    print '     %s | %s | %s' % (board[3],board[4],board[5])
    print '    ---+---+---'
    print '     %s | %s | %s' % (board[6],board[7],board[8])
    print

def make_move(turn_num, player_num, player_sym, player_mark): # Adds a validated move to board
    print "Turn: %s" % turn_num
    print "Player: %s - %s (%s)" % (player_num, player_sym, player_mark)
    chosen_move = raw_input("What is your move, 1 - 9? ")

    try:
        chosen_move = int(chosen_move)

    except ValueError:
        message_box("Validation error: please pick a number, 1 - 9:")
        make_move(turn_num, player_num, player_sym, player_mark)

    else:
        if chosen_move <  1 or chosen_move > 9:
            message_box("Validation error: please pick a number, 1 - 9:")
            make_move(turn_num, player_num, player_sym, player_mark)
        elif board[chosen_move - 1] == "X" or board[chosen_move - 1] == "0" :
            message_box("Validation error: already taken, please pick a different number, 1-9:")
            make_move(turn_num, player_num, player_sym, player_mark)
        else:
            board[chosen_move - 1] = player_mark

def end_game(turn_num, player_num, player_sym, player_mark): # If the game is won or a draw, ends the game
    global ender
    if (board[0] == player_mark and board[1] == player_mark and board[2] == player_mark) or \
       (board[3] == player_mark and board[4] == player_mark and board[5] == player_mark) or \
       (board[6] == player_mark and board[7] == player_mark and board[8] == player_mark) or \
       (board[0] == player_mark and board[3] == player_mark and board[6] == player_mark) or \
       (board[1] == player_mark and board[4] == player_mark and board[7] == player_mark) or \
       (board[2] == player_mark and board[5] == player_mark and board[8] == player_mark) or \
       (board[0] == player_mark and board[4] == player_mark and board[8] == player_mark) or \
       (board[2] == player_mark and board[4] == player_mark and board[6] == player_mark): # Winning move has been made, ends the game
        print
        print '-    G A M E  O V E R'
        print '-    Player %s - %s (%s), WINS.' % (player_num, player_sym, player_mark)
        ender = True

    if turn_num > 9: # Game is a draw, ends the game
        print
        print '-    G A M E  O V E R'
        print '-         D R A W'
        ender = True

#### Programme ####

message_box("N O U G H T S  and  C R O S S E S")
print
print 'Player 1 - crosses (X).'
print 'Player 2 - noughts (O).'
print 'Get a winning line (horizontal, vertical or diagonal) of 3 Noughts or 3 Crosses.'
print 'Use the number grid to pick your move:'
print 
print '     1 | 2 | 3'
print '    ---+---+---'
print '     4 | 5 | 6'
print '    ---+---+---'
print '     7 | 8 | 9'
print 

while ender != True:

    if turn_num %2 == 0:
        player_num = 2
        player_sym = "noughts"
        player_mark = "0"
    else:
        player_num = 1
        player_sym = "crosses"
        player_mark = "X"

    make_move(turn_num, player_num, player_sym, player_mark)
    display_board(board)
    turn_num += 1    
    end_game(turn_num, player_num, player_sym, player_mark)
\$\endgroup\$
6
\$\begingroup\$

This is good code for a rusty coder. Especially a rusty Fortran coder! Congratulations!

First, the experience.

I played it, and had this on screen:

Turn: 1
Player: 1 - crosses (X)
What is your move, 1 - 9? 1
     X |   |
    ---+---+---
       |   |
    ---+---+---
       |   |

Turn: 2
Player: 2 - noughts (0)
What is your move, 1 - 9? 1

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-    Validation error: already taken, please pick a different number, 1-9:
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Turn: 2
Player: 2 - noughts (0)
What is your move, 1 - 9? 5

I'd suggest that you add one, or maybe better two blank lines above the grid when you draw it. This would provide a better edge to the top of the grid, and (2 lines) suggest that the grid belongs to the text below, rather than the text above.

Also, you might consider filling in the grid, or a second copy of the grid, with the available numbers not yet chosen. That would make it easier to play. At the very least, print the grid with available numbers after an invalid selection is made.

Next, the code.

Globals

You rely on global variables too much. I don't know why you have chosen_move at global scope - it's only used locally in one function. The turn_num, board, and ender variables don't need to be global. They can be passed as parameters, or computed, or returned as results into local variables.

Packaging

You really should package the "game" into one more high-level function. There's a standard Python idiom that applies here:

if __name__ == '__main__':
    main()

You can replace main() with play_game() or whatever function you like. But it will make automated testing easier if loading your module doesn't automatically trigger a game.

Scope

You are accessing data as a slightly higher level than I'm comfortable with. I think this will make things tougher for you when you try to provide an automated opponent. For example:

if turn_num %2 == 0:
    player_num = 2
    player_sym = "noughts"
    player_mark = "0"
else:
    player_num = 1
    player_sym = "crosses"
    player_mark = "X"

make_move(turn_num, player_num, player_sym, player_mark)

Given that the player data is a function of the turn number, there's no reason to pass the player data into the make_move function. Instead, defer the computation of player data as long as possible. Just pass the turn number and board into the make_move function:

make_move(turn_num, board)

Now you don't need a global board but can use the parameter instead.

Similarly, you are computing/updating ender in the end_game() function, but this can and should be just a boolean return value:

def end_game(board):
    """Return True if the game is over (win or draw), else False."""

    if ...:
        return True
    else:
        return False

Then in your game loop:

while not end_game(board):
    make_move(turn_num, board)
    display_board(board)
    turn_num += 1

Fiddly bits

  1. In display_board I think I would have used a multi-line string, so that I could just print '''...''' % tuple(board).

    This lack of laziness on your part is common - there are a lot of shortcuts, such as list slices, that you aren't taking.

  2. In make_move you are using recursion rather than a loop to handle errors in input. This is probably okay in a game for yourself, but in more advanced code this is a no-no. It's possible to exhaust the stack by typing in stupid answers over and over. It's not possible to exhaust a while loop. You should prefer that approach.

  3. In make_move you repeatedly compute chosen_move - 1. This is many possible sources of error. Instead, just perform the subtraction one time, and move on.

  4. In end_game you have the global board, which I have mentioned. Also, you are not being lazy in your checks for winning combinations. Instead of this:

    (board[0] == player_mark and board[1] == player_mark and board[2] == player_mark) or \
    (board[3] == player_mark and board[4] == player_mark and board[5] == player_mark) or \
    

    I suggest you try building a list of tuples of the indexes that could win, and using the all builtin:

    winners = ((0,1,2), (3,4,5), (6,7,8), (0,3,6), ... )
    
    for iii in winners:
        if all(board[i] == player_mark for i in iii):
            # Winner, winner, chicken dinner!
    

    For extra points, see the any builtin and get rid of the for loop. (Actually, maybe don't. It's technically valid, but it runs the risk of making your code too hard to read.)

  5. Note that there's no reason to check the turn_num in the end_game function. You can simply check if there are any empty slots in the board:

    if " " not in board:
        # Game over - board is full. Tie?
    
\$\endgroup\$
  • \$\begingroup\$ Fantastic feedback, you clearly put a lot of effort into helping - much appreciated! I will attempt your recommendations and repost ... tbc ... \$\endgroup\$ – D.Innes Dec 15 '17 at 9:55

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.