4
\$\begingroup\$

I wrote this simple command line interface tic tac toe game with unbeatable AI with unittests. Looking for any suggestions how to improve it.

Game:

#views.py


class GameView:

    @property
    def intro(self):
        return '\nTic Tac Toe\n'

    def newlines(self, amount=1):
        return '\n' * amount

    @property
    def number_of_players(self):
        return 'Enter number of players (1-2): '

    @property
    def number_of_players_error(self):
        return '\nPlease enter 1 or 2'

    def board(self, board):
        return '''
        ╔═══╦═══╦═══╗
        ║ {0} ║ {1} ║ {2} ║ 
        ╠═══╬═══╬═══╣
        ║ {3} ║ {4} ║ {5} ║ 
        ╠═══╬═══╬═══╣
        ║ {6} ║ {7} ║ {8} ║ 
        ╚═══╩═══╩═══╝
        '''.format(*board)

    def win_player(self, player):
        return 'Player {} won!'.format(player)

    @property
    def draw(self):
        return '\nGame ended in draw.'

    @property
    def play_again(self):
        return '\nPlay again? (Y/n): '

    def next_move(self, player, move=''):
        return 'Player {}: {}'.format(player, move)

    @property
    def move_not_valid(self):
        return 'Not a valid move'

# controllers.py

from models import Game
from views import GameView

class GameController:

    def __init__(self):
        self.game = Game()
        self.view = GameView()

    def run(self):

        print(self.view.intro)

        while(True):
            players = input(self.view.number_of_players)
            if players == "1":
                self.play(
                    HumanController(player=1), 
                    ComputerController(player=2)
                )
            elif players == "2":
                self.play(
                    HumanController(player=1), 
                    HumanController(player=2)
                )
            else:
                print(self.view.number_of_players_error)
                continue
            break

        self.play_again()

    def play_again(self):
        resp = input(self.view.play_again)
        if resp != 'n':
            self.game = Game()
            print(self.view.newlines())
            self.run()

    def play(self, player1, player2):

        self.display_board

        for i in range(9):

            if i % 2 == 0:
                player1.move(self.game)
            else:
                player2.move(self.game)

                self.display_board

            if self.game.is_won():
                return self.game_results(player1, player2)

    def game_results(self, player1, player2):

        if self.game.winner:
            if player1.marker == self.game.winner:
                print(self.view.win_player(player=1))
            elif player2.marker == self.game.winner:
                print(self.view.win_player(player=2))
        else:
            print(self.view.draw)

    @property
    def display_board(self):
        print(self.view.board(self.game.board_positions))

class PlayerController:
    player = None

    def __init__(self, player):
        self.player = player
        self.view = GameView()

        if player == 1:
            self.marker = 'X'
            self.opponent = 'O'
        else:
            self.marker = 'O'
            self.opponent = 'X'

    def move(self, game):
        raise NotImplementedError

class HumanController(PlayerController):

    def move(self, game):

        while True:
            move = input(self.view.next_move(self.player))
            try:
                move = int(move) - 1
            except:
                move = -1

            if move not in game.available_positions:
                print(self.view.move_not_valid)
            else:
                break

        game.mark(self.marker, move)

class ComputerController(PlayerController):

    def move(self, game):
        move, _ = game.maximized(self.marker, self.opponent)
        game.mark(self.marker, move)
        print(self.view.next_move(self.player, move + 1))

# models.py

class Game:

    winnable_positions = [
        (0,1,2),
        (3,4,5),
        (6,7,8),
        (0,3,6),
        (1,4,7),
        (2,5,8),
        (0,4,8),
        (2,4,6)
    ]

    def __init__(self):
        self.board = [None for _ in range(9)]
        self.moves = []
        self.winner = None

    @property
    def board_positions(self):
        return [i + 1 if not x else x for i, x in enumerate(self.board)]

    @property
    def available_positions(self):
        return [i for i, v in enumerate(self.board) if not v]

    def mark(self, marker, pos):
        self.moves.append(pos)
        self.board[pos] = marker

    @property
    def undo_move(self):
        self.winner = None
        self.board[self.moves.pop()] = None

    def is_won(self):

        board = self.board

        if None not in self.board:
            self.winner = None
            return True

        for a, b, c in self.winnable_positions:
            if board[a] and board[a] == board[b] == board[c]:
                self.winner = board[a]
                return True

        return False

    def maximized(self, marker, opponent):
        best_score, best_move = None, None

        for move in self.available_positions:
            self.mark(marker, move)

            if self.is_won():
                score = self.score(marker, opponent)
            else:
                _, score = self.minimized(marker, opponent)

            self.undo_move

            if best_score == None or score > best_score:
                best_score, best_move = score, move

        return best_move, best_score

    def minimized(self, marker, opponent):
        best_score, best_move = None, None

        for move in self.available_positions:
            self.mark(opponent, move)

            if self.is_won():
                score = self.score(marker, opponent)
            else:
                _, score = self.maximized(marker, opponent)

            self.undo_move

            if best_score == None or score < best_score:
                best_score, best_move = score, move

        return best_move, best_score

    def score(self, marker, opponent):
        if self.is_won():
            if self.winner == marker:
                return 1

            elif self.winner == opponent:
                return -1
        return 0

Unittests:

# test_views.py

import unittest
from views import GameView

class TestGameView(unittest.TestCase):

    def setUp(self):
        self.view = GameView()

    def test_board(self):
        board = [1, 'O', 'O', 'X', 5, 'X', 7, 8, 9]
        self.assertEqual(self.view.board(board), '''
        ╔═══╦═══╦═══╗
        ║ 1 ║ O ║ O ║ 
        ╠═══╬═══╬═══╣
        ║ X ║ 5 ║ X ║ 
        ╠═══╬═══╬═══╣
        ║ 7 ║ 8 ║ 9 ║ 
        ╚═══╩═══╩═══╝
        ''')

    def test_new_lines(self):
        self.assertEqual(self.view.newlines(), '\n')
        self.assertEqual(self.view.newlines(3), '\n\n\n')

    def test_win_player(self):
        self.assertEqual(self.view.win_player(1), 'Player 1 won!')
        self.assertEqual(self.view.win_player(2), 'Player 2 won!')

    def test_next_move(self):
        self.assertEqual(self.view.next_move(1, 1), 'Player 1: 1')
        self.assertEqual(self.view.next_move(1, 5), 'Player 1: 5')
        self.assertEqual(self.view.next_move(2, 6), 'Player 2: 6')

# test_controllers.py

import unittest
from unittest.mock import patch
from models import Game
from controllers import *
import io

class TestGameControllerPlayAgain(unittest.TestCase):
    """ GameController.play_again """

    @patch('sys.stdout', new_callable=io.StringIO)
    def assert_stdout(self, expected_output, stdout):
        game = GameController()
        try:
            game.play_again()
        except StopIteration:
            pass
        self.assertIn(expected_output, stdout.getvalue())

    @patch('builtins.input', side_effect=['Y'])
    def test_play_again__yes(self, input):
        self.assert_stdout('Tic Tac Toe')
        self.assertTrue(input.called)

    @patch('builtins.input', side_effect=['n'])
    def test_play_again__no(self, input):
        self.assert_stdout('')
        self.assertTrue(input.called)

    @patch('builtins.input', side_effect=[''])
    def test_play_again__default(self, input):
        self.assert_stdout('Tic Tac Toe')
        self.assertTrue(input.called)

class TestGameControllerPlay(unittest.TestCase):
    """ GameController.play """

    @patch('sys.stdout', new_callable=io.StringIO)
    def assert_stdout(self, player1, player2, expected_output, stdout):
        game = GameController()
        try:
            game.play(player1, player2)
        except StopIteration:
            pass
        self.assertIn(expected_output, stdout.getvalue())

    @patch('builtins.input', side_effect=[1, 4, 2, 5, 3])
    def test_play__human(self, input):
        player1 = HumanController(player=1)
        player2 = HumanController(player=2)
        self.assert_stdout(player1, player2, 'Player 1 won!')
        self.assertTrue(input.called)

    @patch('builtins.input', side_effect=[5, 6, 7, 2, 9])
    def test_play__computer_draw(self, input):
        player1 = HumanController(player=1)
        player2 = ComputerController(player=2)
        self.assert_stdout(player1, player2, 'Game ended in draw')
        self.assertTrue(input.called)

    @patch('builtins.input', side_effect=[1, 2, 4])
    def test_play__computer_win(self, input):
        player1 = HumanController(player=1)
        player2 = ComputerController(player=2)
        self.assert_stdout(player1, player2, 'Player 2 won!')
        self.assertTrue(input.called)

@patch('sys.stdout', new_callable=io.StringIO)
class TestGameControllerGameResults(unittest.TestCase):
    """ GameController.game_results """

    def setUp(self):
        self.game = Game()
        self.controller = GameController()
        self.player1 = HumanController(player=1)
        self.player2 = HumanController(player=2)

    def test_draw(self, stdout):
        self.controller.game_results(self.player1, self.player2)
        self.assertIn('Game ended in draw', stdout.getvalue())

    def test_win_player1(self, stdout):
        self.controller.game.winner = 'X'
        self.player1.marker = 'X'
        self.controller.game_results(self.player1, self.player2)
        self.assertIn('Player 1 won', stdout.getvalue())

    def test_win_player2(self, stdout):
        self.controller.game.winner = 'O'
        self.player2.marker = 'O'
        self.controller.game_results(self.player1, self.player2)
        self.assertIn('Player 2 won', stdout.getvalue())


class TestPlayerController(unittest.TestCase):
    """ PlayerController """

    def test_player1(self):
        controller = PlayerController(player=1)
        self.assertEqual(controller.player, 1)
        self.assertEqual(controller.marker, 'X')
        self.assertEqual(controller.opponent, 'O')

    def test_player2(self):
        controller = PlayerController(player=2)
        self.assertEqual(controller.player, 2)
        self.assertEqual(controller.marker, 'O')
        self.assertEqual(controller.opponent, 'X')

    def test_move(self):
        game = Game()
        controller = PlayerController(player=1)
        with self.assertRaises(NotImplementedError):
            controller.move(game)

# test_models.py

import unittest
from unittest.mock import patch
from models import Game
import sys

class TestGame(unittest.TestCase):

    def setUp(self):
        self.game = Game()

    def test_board_positions(self):
        self.game.board = [None, 'X', 'O', None]
        self.assertEqual(self.game.board_positions, [1, 'X', 'O', 4])

        self.game.board = ['X', 'O', None, None]
        self.assertEqual(self.game.board_positions, ['X', 'O', 3, 4])


    def test_available_positions(self):
        self.game.board = [None, 'X', 'O', None]
        self.assertEqual(self.game.available_positions, [0, 3])

        self.game.board = ['X', 'O', None, None]
        self.assertEqual(self.game.available_positions, [2, 3])

    def test_mark(self):
        game = self.game
        game.mark('O', 2)
        self.assertEqual(game.moves, [2])
        self.assertEqual(game.board.count('O'), 1)
        self.assertEqual(game.board.count('X'), 0)

        self.game.mark('X', 5)
        self.assertEqual(game.moves, [2, 5])
        self.assertEqual(game.board.count('O'), 1)
        self.assertEqual(game.board.count('X'), 1)

    def test_undo_move(self):
        game = self.game
        game.board = [None, 'O', None, 'X']
        game.moves = [1, 3]
        game.winner = 'O'

        game.undo_move
        self.assertIsNone(game.winner)
        self.assertEqual(game.moves, [1])
        self.assertEqual(game.board, [None, 'O', None, None])

        game.undo_move
        self.assertIsNone(game.winner)
        self.assertEqual(game.moves, [])
        self.assertEqual(game.board, [None, None, None, None])

    def test_is_won__false(self):
        self.game.board = ['X', 'X', None, 'X', None, None, None, None, None]
        self.assertFalse(self.game.is_won())

    def test_is_won__true(self):
        self.game.board = ['X', 'X', 'X', None, None, None, None, None, None]
        self.assertTrue(self.game.is_won())

    def test_is_won__full_board(self):
        self.game.board = ['X'] * 9
        self.assertTrue(self.game.is_won())

    def test_maximized(self):
        self.game.board = ['X', 'X', 'X', None, None, None, None, None, None]
        self.assertEqual(self.game.maximized('X', 'O'), (3, 1))

    def test_minimized(self):
        self.game.board = ['X', 'X', 'X', 'O', 'O', None, None, None, None]
        self.assertEqual(self.game.minimized('X', 'O'), (5, 1))

    def test_score(self):
        self.game.board = ['X', 'X', 'X', None, None, None, None, None, None]
        self.assertEqual(self.game.score(marker='X', opponent='O'), 1)

        self.game.board = ['O', 'O', 'O', None, None, None, None, None, None]
        self.assertEqual(self.game.score(marker='X', opponent='O'), -1)

        self.game.board = ['X', 'X', None, None, None, None, None, None, None]
        self.assertEqual(self.game.score(marker='X', opponent='O'), 0)
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

I'm unclear on why GameView has a list of @property functions that are just static strings. This might in theory be useful if you have to support i18n, but I doubt that's the intent here. As such, intro, number_of_players, number_of_players_error, draw, etc. don't need to be properties; that's just clutter.

Another quirk of GameView is that self is never used, so it doesn't deserve to be a class. At most, if you needed to keep those functions but wanted them in a namespace, you would move them into the global scope and use views as the namespace.

To make your board initialization literal easier to read, you can write

board = [1, 'O', 'O', 'X', 5, 'X', 7, 8, 9]

as

board = ( 1 ,'O', 'O',
         'X', 5 , 'X',
          7 , 8 ,  9 )

That particular set of data is problematic. You're mixing up "display" data (i.e. 'X') with "logical" data (i.e. 7). The format of your board should be such that it isn't translated to display characters until it hits the view.

You have tests! Awesome!

Your player number input loop is awkward. I'd suggest rewriting it as:

while True:
    try:
        players = int(input(self.view.number_of_players))
        if not (1 <= players <= 2):
            raise ValueError()
        break
    except ValueError:
        print(self.view.number_of_players_error)

if players == 1:
    type_2 = ComputerController
else:
    type_2 = HumanController
self.play(HumanController(player=1),
          type_2(player=2))

This:

if resp != 'n':

should lower-case resp before the check.

This:

self.display_board

is nasty. You're accessing a property, relying on its side-effect to do a print. Properties should be simple value-returning functions. Make that a normal function.

Given this:

class PlayerController:
    player = None

I'm unclear on why you declared player at the class level. That can probably go away.

This:

raise NotImplementedError

doesn't do what you think it does. You need to add () for that error type to be instantiated.

This:

        try:
            move = int(move) - 1
        except:
            move = -1

should catch ValueError; the exception clause is too broad.

This:

self.board = [None for _ in range(9)]

can just be

self.board = [None]*9

This

i + 1 if not x else x

can just be

x or (i + 1)
\$\endgroup\$
3
  • \$\begingroup\$ raise Exception works also without the (). \$\endgroup\$
    – Graipher
    Commented Dec 30, 2018 at 13:23
  • 1
    \$\begingroup\$ @Graphier Apparently it works but it's discouraged. Have a read through PEP317 - python.org/dev/peps/pep-0317 - even though it was rejected, it captures the common sentiment. \$\endgroup\$
    – Reinderien
    Commented Dec 30, 2018 at 14:57
  • 1
    \$\begingroup\$ Thanks, interesting read! It seems like I'm in the camp thinking it looks a bit ugly with the empty parentheses, but YMMV. It would be nice if the "correct" way was mentioned in PEP8... \$\endgroup\$
    – Graipher
    Commented Dec 30, 2018 at 15:14

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