3
\$\begingroup\$

I made a simply user vs. computer game on Tic Tac Toe. I used Object Oriented Programming to increase my understanding of this programming paradigm. Can you criticize any design flaws I have in my code?

#!/usr/bin/env python3
# -*- coding: utf-8 -*-
"""
Created on Sat Dec 15 20:56:48 2018

@author: von-vic
"""
import random
from itertools import combinations

class Board(object):

    def __init__(self):
        self.board = {x:None for x in (7,8,9,4,5,6,1,2,3)}

    def display(self):
        """
        Displays tic tac toe board
        """
        d_board = '\nTIC TAC TOE:\n'
        for pos, obj in self.board.items():
            if obj == None:
                d_board += '_'
            elif obj == 'X':
                d_board += 'X'
            elif obj == 'O':
                d_board += 'O'

            if pos%3 == 0:
                d_board += '\n'
        print(d_board)

    def getAvailable(self):
        """
        Returns available positions
        """
        available = []
        for pos, obj in self.board.items():
            if obj == None:
                available.append(pos)
        return available

class Tic_Tac_Toe(Board):
    pieces = ['O', 'X']

    def __init__(self):
        super().__init__()
        self.piece = Tic_Tac_Toe.pieces.pop(random.choice([0,1]))
        self.cp_piece = Tic_Tac_Toe.pieces[0]

    def user_setPiece(self, position):
        """
        Position parameter denoted by a number on the keypad (1-9)
        """
        self.board[position] = self.piece

    def user_getPiece(self):
        return self.piece

    def cp_setPiece(self):
        self.board[random.choice(self.getAvailable())] = self.cp_piece

    def cp_getPiece(self):
        return self.cp_piece

    def checkWin(self, player):
        """
        Checks if move by either the user or computer results in a win
        """
        def at_least_one(A, B):
            for i in A:
                for j in B:
                    if i == j:
                        return True
            return False

        win_patterns = [(1,2,3),(4,5,6),(7,8,9),
                        (1,4,7),(2,5,8),(3,6,9),
                        (3,5,7),(1,5,9)]
        spots = [k for k, v in self.board.items() if v == player]
        spots.sort()
        player_combinations = list(combinations(spots,3))
        if at_least_one(player_combinations, win_patterns) == True:
            return True
        return False

    def checkFullBoard(self):
        if None not in self.board.values():
            self.display()
            print('Draw! Game board full!')
            return True
        return False

#---------

def main():
    # Setup game
    game = Tic_Tac_Toe()
    input('Hello user! Welcome to Tic Tac Toe! Press any key to continue')

    if game.user_getPiece() == 'X':
        print('You are X. You are going first.')
    else:
        print('You are O. You are going second.')
        game.cp_setPiece()

    # Main game loop
    while True:
        game.display()
        position = input('Use the number pad on the lefthand side of your keyboard\nto select your position (1-9):')

        try:
            position = int(position)
            if position in range(1,10):
                if position in game.getAvailable():
                    game.user_setPiece(position)        
                else:
                    print('----Please input an available position.')
                    continue
            else:
                print('----Please input a number between 1 and 9.') 

        except ValueError:
            print('----Please input a number.')
            continue

        # FOR USER
        # Check for win
        if game.checkWin(game.user_getPiece()) == True:
            game.display()
            print('Congratulations! You win!')
            break

        # Check for full board
        if game.checkFullBoard() == True:
            break

        # FOR COMPUTER
        game.cp_setPiece()
        # Check for win
        if game.checkWin(game.cp_getPiece()) == True:
            game.display()
            print('Sorry. You lost.')
            break

        # Check for full board
        if game.checkFullBoard() == True:
            break

if __name__ == '__main__':
    main()
\$\endgroup\$
3
\$\begingroup\$

First of all, you're off to a good start. You have some comments, but it's important to be rigorous and consistent with your documentation. Uisdean's points about PEP8 styling are valid, but don't address the organization of your code. Additionally, stylistic changes can largely be fixed by automatic tools (e.g. Black).

From an OOP point of view, it's important to understand the semantics of your class structure. From what you have now, Board is a base class, and because Tic_Tac_Toe subclasses Board, semantically this says that Tic_Tac_Toe is a variety of Board. The functions in Tic_Tac_Toe allow it to manipulate itself with moves, and then you have a main loop to run the logic. This is a good start, but I think we can do better.

My understanding is that Tic Tac Toe is a type of game, not a type of board, but it does use a board as an internal state. Also, the game Tic Tac Toe doesn't manipulate itself. Instead, players manipulate the state of the game by making taking turns.

Therefore, my advice about how to organize this code from a object oriented point of view would be to create classes like (parenthesis indicate a class' superclass): Game, TicTacToe(Game), Board, Player, Human(Player), Computer(Player), Turn, and Move. Then, each Game would have a set of Turns, each with a corresponding Move and Player. Moves would affect the state of the Board within the Game instance. The kind of Moves that are allowed depend on the rules defined by the subclasses of Game (TicTacToe in this instance). Depending on the kind of Player, the Move for each Player's Turn could be generated automatically (for a Computer), or through keyboard/mouse input (for a Human).

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

The first things which pop up are violations of the pep8 coding guidelines of python, for example

self.board = {x:None for x in (7,8,9,4,5,6,1,2,3)}

should be

self.board = {x:None for x in (7, 8, 9, 4, 5, 6, 1, 2, 3)}

because of "missing white space after ','" There are more violations like naming conventions

  • Class names should be CamelCase
  • Function names should be lower_case and snake_case
  • As should variables

it is more pythonian to write

    if at_least_one(player_combinations, win_patterns):
        return True

instead of

    if at_least_one(player_combinations, win_patterns) == True:
        return True

And in the same function the naming can be improved: If you read the statement it doesn't convey what you are testing for: "if at_least_one" The tipp I can give here: Write your code like an essay.

The last thing I want to mention is

if obj == None:

it would be more pythonic to write

if obj is None
\$\endgroup\$
0

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.