I'm working on one easy question (Leetcode 1275. Find Winner on a Tic Tac Toe Game) and am wondering how I can make the code even cleaner.

class Solution:
    def tictactoe(self, moves: List[List[int]]) -> str:
        from collections import Counter

        def is_win(pos: List[List[int]]) -> bool:
            pos = set(tuple(i) for i in pos)
            d1, d2 = set([(0, 0), (1, 1), (2, 2)]), set([(0, 2), (1, 1), (2, 0)])
            x = Counter(i for i, _ in pos)
            y = Counter(i for _, i in pos)
            d1 -= set(pos)
            d2 -= set(pos)
            return (3 in x.values()) or (3 in y.values()) or not d1 or not d2

        a, b = moves[::2], moves[1::2]
        A, B = is_win(a), is_win(b)
        if A:
            return 'A'
        elif B:
            return 'B'
        elif not A and not B and (len(moves) == 9):
            return 'Draw'
        return 'Pending'
  • \$\begingroup\$ Can you add a context of calling your function tictactoe with sample value for moves ? \$\endgroup\$ – RomanPerekhrest Dec 9 '19 at 9:43
  • \$\begingroup\$ Yep, using List[int] instead of the more obvious Tuple[int, int] for (row, col) is definitely confusing. Leetcode should have defined better types here. \$\endgroup\$ – Roland Illig Dec 9 '19 at 10:19

The second half of your code is inefficient, as it computers is_win(b) unnecessarily. It can be written as:

    if is_win(moves[::2]):
        return 'A'
    elif is_win(moves[1::2]):
        return 'B'
    elif len(moves) == 9:
        return 'Draw'
        return 'Pending'

This removes the redundant not a and not b, the redundant parentheses and the redundant computation of is_win in case A has won already.

When I saw is_win for the first time, I didn't understand it.

  • Maybe my chances had been better if you renamed d1 to diag1.
  • You could also write diag1 and diag2 on separate lines, to align their coordinates vertically.
  • You could also inline the diag1 and diag2 so that you don't need a name for them at all.

I'm thinking of:

def is_win(pos: List[List[int]]) -> bool:
    pos = set(tuple(i) for i in pos)
    horiz = 3 in Counter(r for r, _ in pos).values()
    verti = 3 in Counter(c for _, c in pos).values()
    diag1 = not (set([(0, 0), (1, 1), (2, 2)]) - pos)
    diag2 = not (set([(0, 2), (1, 1), (2, 0)]) - pos)
    return horiz or verti or diag1 or diag2

To avoid unnecessary computations here as well, you can let your IDE inline the variables except for pos.

Instead of not (set1 - set2), the expression set1.issubset(set2) is clearer because it avoids the confusing not:

    diag1 = {(0, 0), (1, 1), (2, 2)}.issubset(pos)
    # or:
    diag1 = {(0, 0), (1, 1), (2, 2)} <= pos

Or you could make checking the diagonals similar to horizontal and vertical:

    diag1 = 3 in Counter(r - c for r, c in pos).values()
    diag2 = 3 in Counter(r + c for r, c in pos).values()
  • \$\begingroup\$ it is good idea to use multiple list comprehensive other than just use one for loop? \$\endgroup\$ – jacobcan118 Dec 9 '19 at 17:32

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.