2
\$\begingroup\$
def tictactoe():
    Board = [1,2,3,4,5,6,7,8,9]
    w = True
    PlayerX = []
    PlayerO = []
    while len(PlayerO) < 9:
        while True:
            z = int(input("Player X: Select a place (1-9)\n"))
            if z in Board:
                Board[z-1] = []
                PlayerX.append(z)
                break
            else:
                print("Place not available! Choose another X")
        while True:
            y = int(input("Player O: Select a place (1-9)\n"))
            if y in Board:
                Board[y-1] = []
                PlayerO.append(y)
                break
            else:
                print("Place not available! Choose another O")
        if len(PlayerX) > 2:
            if set([1,2,3]).issubset(set(PlayerX)) or set([4,5,6]).issubset(set(PlayerX)) or set([7,8,9]).issubset(set(PlayerX)) or set([1,4,7]).issubset(set(PlayerX)) or set([2,5,8]).issubset(set(PlayerX)) or set([3,6,9]).issubset(set(PlayerX)) or set([1,5,9]).issubset(set(PlayerX)) or set([3,5,7]).issubset(set(PlayerX)):
                print("Player X won!")
                w = False
                break
            elif set([1,2,3]).issubset(set(PlayerO)) or set([4,5,6]).issubset(set(PlayerO)) or set([7,8,9]).issubset(set(PlayerO)) or set([1,4,7]).issubset(set(PlayerO)) or set([2,5,8]).issubset(set(PlayerO)) or set([3,6,9]).issubset(set(PlayerO)) or set([1,5,9]).issubset(set(PlayerO)) or set([3,5,7]).issubset(set(PlayerO)):
                print("Player O won!")
                w = False
                break
    if w is True:
        print("Tie")
    if input("Play Again (y/n)?\n") == "y":
        tictactoe()
tictactoe()

I'm trying to make this code as efficient as possible. I'm new to Python and kinda limited with the knowledge I have. How can I simplify the code and especially the line shown below?

Numbered spots (1-9)

if set([1,2,3]).issubset(set(PlayerX)) or set([4,5,6]).issubset(set(PlayerX)) or set([7,8,9]).issubset(set(PlayerX)) or set([1,4,7]).issubset(set(PlayerX)) or set([2,5,8]).issubset(set(PlayerX)) or set([3,6,9]).issubset(set(PlayerX)) or set([1,5,9]).issubset(set(PlayerX)) or set([3,5,7]).issubset(set(PlayerX)):
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Define "efficient". Do you mean in regards to runtime performance, development time, maintenance complexity, ...? \$\endgroup\$
    – Flater
    Commented Mar 12, 2020 at 12:11

2 Answers 2

5
\$\begingroup\$

I guess from efficiency point of view this is fine, even that last line. Efficiency points:

  • You create player variable as list and then keep converting to set over and over again, why not have it as set from beginning?

From readability and maintenance point of view, you are having lots of duplicities.

  • Player moves are exactly same except for message and variable, you can extract that to function.
  • Same goes for checking if a player won.
  • I don't like variable w. Why not make variable winner instead and save name of winner into it? Related to extracting if player won to separate function.

And for your last line - you can put all possible winning combinations into array or tupple and then check that in cycle (you get the idea):

winning_combinations = [
[1, 2, 3],
[4, 5, 6],
...
]
\$\endgroup\$
2
\$\begingroup\$

In addition to what @K.H. already mentioned:

Comments to the code

  • Things will get simplified if you represent the Board as a set of numbers
Board = set([1,2,3,4,5,6,7,8,9])

Then Board.remove(z) would look better than doing manipulations with indices and assigning an empty list (Board[z-1] = [])

  • The condition below is not really used:
while len(PlayerO) < 9:

Can the length of Player0 ever be more or equal to 9?

  • You can simplify if w is True: -> if w:

General comment about variable naming

I would like also to pay your attention to variable naming. It might seem insignificant for a small program like yours, but it is important. After some time you are going to forget which information variables like w or Board actually hold, and those who read your program won't be able to understand it right away either. So it will take more time to understand your code.

For example, you could rename your variables as follows:

Board -> free_places_on_board

Player0 -> player_0_selected_places

w -> tie

z -> selected_place

Although these names are longer (shouldn't be a problem since all modern IDEs provide code completion), but they give much more information, and the code reads much easier, just like the English language:

selected_place = int(input("Player O: Select a place (1-9)\n"))
if selected_place in free_places_on_board:
    player_0_selected_places.append(selected_place)

A big advantage of this is that if there's a bug somewhere in your code, it will become obvious:

if not w:         # not obvious
    print("Tie")
if not tie:       # now the statement does not make sense -> the bug becomes obvious 
    print("Tie")
\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.