3
\$\begingroup\$

I'm trying to get more coding experience and decided to start small with a basic noughts & crosses game.

Any feedback is welcome and below are the items I think I should add

  • a print out of an actual grid not the array
  • more error catching ie user enters T instead of X or O
  • potential position recommendations
  • more comments in the code

import random

def grid(n):
    
    board = []
    
    for i in range(n):
        board.append("-")
    return board


board1 = grid(9)
    


print(board1)
   
p1 = input("Enter your value X or O: ")

if p1 =='X':
    p2 = 'O'
else:
    p2 = 'X'


def check_column(board, p, pos):
    if board[pos + 3*0] == p:
        if board[pos+3*1] == p:        
            if board[pos+3*2] == p:        
                return True
            
def check_row(board, p, pos):
    if board[pos + 0] == p:
        if board[pos + 1] == p:        
            if board[pos + 2] == p:        
                return True

def check_dig1(board, p, pos):
    if board[pos + 0] == p:
        if board[pos + 2] == p:        
            if board[pos + 4] == p:        
                return True


def check_dig2(board, p, pos):
    if board[pos + 0] == p:
        if board[pos + 4] == p:        
            if board[pos + 8] == p:        
                return True

 
def check_win(board, player):
    
    length = len(board)
    #column check
    for i in range(3):
        if check_column(board, player,i):
            print('col')
            return True
        
    for i in range(0,7,3):
        if check_row(board, player,i):
            print('col')
            return True
        
    if check_dig1(board, player, 2):
        print('diag1')
        return True
    
    if check_dig2(board, player, 0):
        print('diag2')
        return True

    return False




while True:
    #val, pos = input("Enter two values: ").split(',')
    position = input("P1 enter your position: ")
    
    board1[int(position)-1] = p1
    
    print(board1)
    
    if check_win(board1,p1):
        print('P1 has won')
        print(board1)
        break
    
    
    if board1.count('X') + board1.count('O')  == 9:
        print("Tie!")
        break
    
    position = input("P2 enter your position: ")
    
    board1[int(position)-1] = p2
    
    print(board1)
    
    if check_win(board1,p2):
        print('P2 has won')
        print(board1)
        break

    if board1.count('X') + board1.count('O')  == 9:
        print("Tie!")
        break

# check_win(board1, 'O')
# print(board1)
\$\endgroup\$
1
  • \$\begingroup\$ A little FYI, if you think my answer improves your code, consider clicking that upwards triangle and the tick mark... \$\endgroup\$ Jun 9 at 14:20

1 Answer 1

2
\$\begingroup\$

Your script does do the job (occasionally), but it is not very efficient, which is understandable as you are a beginner.

  • Do not use a for loop to repeatedly add the same element

Your method to create the grid is very inefficient:

def grid(n):
    
    board = []
    
    for i in range(n):
        board.append("-")
    return board

Don't use it, instead use list comprehension, this:

board = ['-' for i in range(n)]

Does exactly the same thing as your list appending, but is much more efficient, sinces it is translated to optimized C code when executed.

But since board is simply the same element repeated n times, just use list multiplication, which is even faster:

board = ['-'] * n

So the grid function can be written as:

def grid(n):
    return ['-'] * n
  • Don't use nested if statements

Don't use nested if statements.

You have a bunch of functions like this:

def check_column(board, p, pos):
    if board[pos + 3*0] == p:
        if board[pos+3*1] == p:        
            if board[pos+3*2] == p:        
                return True

Do you see that the right-hand operand is all p, and the left-hand operand has a pattern?

Since each stage the index is increased by 3, simply use a for loop to check these conditions. But here you are only check if all conditions are met, you can just use all, which short-circuits (following checks won't be performed after falsy value is encountered).

And, since the maximum index in your grid is 8, your second check will raise index error if pos is greater than 6 and third check will also raise index error is pos is greater than 2.

Instead use this:

def check_column(board, p, pos):
    return all(board[(pos + i) % 9] == p for i in (0, 3, 6))
  • Input validation

You don't do input validation, there's nothing preventing the user from inputting a value that is neither 'O' or 'X', and you automatically assume the input will be valid, so there can be logical errors.

For example, if I input 'x' (Python is case sensitive), according to this:

if p1 =='X':
    p2 = 'O'
else:
    p2 = 'X'

p1 and p2 will both be cross...

Instead use a while loop to repetitively get user input until valid value is entered:

while True:
    p1 = input("Enter your value X or O: ").upper()
    if p1 in ('O', 'X'):
        break

if p1 =='X':
    p2 = 'O'
else:
    p2 = 'X'
\$\endgroup\$
3
  • \$\begingroup\$ [a list comprehension is] much more efficient, since it is translated to optimized C code when executed is not based in fact. I encourage you to do some reading and testing on this topic. This kind of performance claim is impossible to make without profiling, and I think you misunderstand the mechanism by which JIT is done on a for-loop versus a comprehension. It doesn't take long to find existing examples of performance comparisons that demonstrate the opposite of your claim. \$\endgroup\$
    – Reinderien
    Jun 9 at 12:08
  • \$\begingroup\$ More broadly: this is not the place to attempt micro-optimisation, and is an excellent example of premature optimisation: the code is not CPU-bound, and so overwhelmingly, legibility and correctness are much more important. \$\endgroup\$
    – Reinderien
    Jun 9 at 12:10
  • \$\begingroup\$ Thanks all for the comments, agree Reinderien I do want to improve legibility and correctness but will keep note on alternative methods/minimum lines of code needed to achieve the same goal \$\endgroup\$
    – MM360
    Jun 9 at 14:12

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.