4
\$\begingroup\$

I wrote this program in Python 3.I would like to know if there is some more functionality that could be added/removed/is missing.Any comments would be helpful!

#Steps from user :
"""
Enter Input
P1 and P2
Each time for input will be preceded by a printed board.
The game will continue until an input matches a win combination.
"""
win_combinations = [
    [1,2,3],
    [4,5,6],
    [7,8,9],
    [1,4,7],
    [2,5,8],
    [3,6,9],
    [1,5,9],
    [3,5,7]]

tot_list = [1,2,3,4,5,6,7,8,9]

def board():
    print ("{}|{}|{}".format(tot_list[0],tot_list[1],tot_list[2]))
    print ("{}|{}|{}".format(tot_list[3],tot_list[4],tot_list[5]))
    print ("{}|{}|{}".format(tot_list[6],tot_list[7],tot_list[8]))


p1 = 0
p2 = 0


def player_input():
    global p1
    global p2
    while True:
        try:
            print("\n\n")
            board()
            print("\n")
            p1 = int(input("P1(X): Enter your Input:"))
            print("\n")
            p2 = int(input("P2(O): Enter your Input:"))
            if 0 < p1 < 10 and 0 < p2 < 10 :
                assign_values(p1,p2)
                break
            print("Re-enter valid input")
        except:
            print("Invalid input")
            continue

def assign_values(player1,player2):
    if type(tot_list[player1-1]) == str() or type(tot_list[player2-1]) == str():
        print("Re-enter valid input")
        player_input()
    if player1 in tot_list:
        tot_list[player1-1] = "X"
    if player2 in tot_list:
        tot_list[player2-1] = "O"


p1_win = False
p2_win = False
tie = False


def default_values():
    global tot_list
    global p1
    global p2
    global p1_win
    global p2_win
    global tie
    tot_list = [1,2,3,4,5,6,7,8,9]
    p1 = 0
    p2 = 0
    p1_win = False
    p2_win = False
    tie  = False



def play():
    global p1_win
    global p2_win
    global tie
    global tot_list
    while True:
        player_input()
        for i in win_combinations:
            if tot_list[i[0]-1] == "X" and tot_list[i[1]-1] == "X" and tot_list[i[2]-1] == "X":
                p1_win = True
                break
            elif tot_list[i[0]-1] == "O" and tot_list[i[1]-1] == "O" and tot_list[i[2]-1] == "O":
                p2_win = True
                break
        if all(isinstance(i,str) for i in tot_list) == True:
            tie == False
        if p1_win==True:
            print("P1 has won!")
            break
        elif p2_win==True:
            print("P2 has won!")
            break
        elif tie == True:
            print("Its a tie!")
            break

    play_again = input("Play Again?(y/n)")
    if play_again == "y":
        default_values()
        play()
    else:
        print("Ciao")
if __name__ == '__main__':
    play()
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

Your code doesn't work like in real life. For example, when I play:

1|2|3
4|5|6
7|8|9

P1(X): Enter your Input:1

P2(O): Enter your Input:1

X|2|3
4|5|6
7|8|9

There is no error generated. Both players cannot request the same location. Also, playing a new game, this scenario:

P2(O): Enter your Input:5

X|O|3
X|O|6
7|8|9    

P1(X): Enter your Input:7

P2(O): Enter your Input:

This scenario P2(O): should never have been reached. Player 2 doesn't get another move after Player 1 wins the game. Recommend fixing these problems.

Onto your code -

Firstly, you have an entry point - that's good, however it doesn't do anything other than call play().

You should move the control loop into the entry point, and remove duplicate code scattered amongst your code. In default_values(), you scatter those pieces across the code.

tot_list = [1, 2, 3, 4, 5, 6, 7, 8, 9]
p1 = 0
p2 = 0
p1_win = False
p2_win = False
tie = False

Yet you have them inside default_values(). Inside your play() function, you have play() in the loop, but the first time you call it, you specify default_values() - notice the inconsistency there?
You should have realized that when you were reviewing your code.

So, we fix that, and this is what we come up with:

if __name__ == '__main__':

    win_combinations = [
        [1, 2, 3],
        [4, 5, 6],
        [7, 8, 9],
        [1, 4, 7],
        [2, 5, 8],
        [3, 6, 9],
        [1, 5, 9],
        [3, 5, 7]]

    while True:
        default_values()
        play()
        choice = input("Play Again? ('n' to exit):")
        if choice == "n":
            break

    print("Ciao")

You also have lots of other issues with your code, but let me give you a few pointers for the rest (I don't want to rewrite your entire code).

if p1_win == True: You don't need to specify "True" as this is the same: if p1_win:

You have global statements all throughout your code. This is bad - and the reason is - you don't know 100% where your variables are being changed. The #1 source of all bugs when coding is when the state changes in variables and the change was unintended. Does that make sense? Please review it by googling.
What you should do is pass variables around. Having a piece of code like:

active_player = not active_player
player_input = get_player_input() 
board_state = update_board(player_input, board_state)
if check_for_a_winner(board_state):
    print("We have a winner! {} wins!".format(active_player))

shows you how we inject the input as well as the current board status into the update_board function, and receive the updated board status back from it. We can then present the current board status into the winner check function.
This examples shows that state changes only in very specific places, and if you're hunting down a bug, you can find it quite simply.

Typically, with several pieces of related state, you'll need an object which can represent all of them at once. I recommend reading a book which explains about classes, for instance having multiple dog and cat classes and how you can represent their attributes.

Please attempt to incorporate these changes into you code and repost your updated solution (with a link to this page) and demonstrate your code working better, with those bugs removed.

Good Luck!

\$\endgroup\$
0
\$\begingroup\$

just appending to @C. Harley. as you are aware of .format, you might want to use it in prints as well

from

print("\n\n")
board()
print("\n")

to

print(""""

{}
""".format(board())
)
\$\endgroup\$
2
  • 1
    \$\begingroup\$ This print statement would ruin your indentation levels, but a fair call on injecting the board print statement. print("\n\n{}\n".format(board()) would be preferred to your snippet, however if we're being proper, the new-lines are specific to printing the board, they belong inside the board() function and not outside of it. \$\endgroup\$
    – C. Harley
    Aug 14, 2018 at 8:04
  • \$\begingroup\$ leaving it here for as an idea springer, but see @C. Harley's comment above \$\endgroup\$ Aug 15, 2018 at 10:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.