4
\$\begingroup\$

Does anyone have suggestions for how to make this code for playing a game of Quarto cleaner and more robust (beyond input error checking, which hasn't been fully fleshed out yet)?

# Enums for quarto.
DRAW = "D"
WIN = "W"
UNDECIDED = "U"
LOSE = "L"
TIE = "T"

class Piece():
    """ 
    This is the piece class.
    """
    attributes = None
    full_name = ""
    abbreviation = ""

    def __init__(self, attributes):
        """ 
        Constructor for our piece class with support for 
        the various pieces in the game of Quarto.
        """
        self.attributes = attributes
        if attributes & 0b0001:
            self.full_name += "Tall"
            self.abbreviation += "T"
        else:
            self.full_name += "Short"
            self.abbreviation += "S"
        if attributes & 0b0010:
            self.full_name += " black"
            self.abbreviation += "B"
        else:
            self.full_name += " white"
            self.abbreviation += "W"
        if attributes & 0b0100:
            self.full_name += " circle"
            self.abbreviation += "C"
        else:
            self.full_name += " square"
            self.abbreviation += "Q"
        if attributes & 0b1000:
            self.full_name += " solid-top"
            self.abbreviation += "D"
        else:
            self.full_name += " hollow-top"
            self.abbreviation += "H"

    def get_attributes(self):
        """ 
        Returns a list of attributes.
        """
        return self.attributes

    def get_piece_name(self):
        """
        Returns the name of the piece.
        """
        return self.full_name

    def get_piece_abbr(self):
        """
        Returns the abbreviation of the piece.
        """
        return self.abbreviation

class Board():
    """
    This is the board class.
    """
    pieces = []
    cols_count = 4
    rows_count = 4
    board = None

    def __init__(self):
        """
        Constructor for our board class. 
        Appends all the pieces of quarto to our array of pieces.
        Implemented by a 4x4 2D array.
        """
        self.board = [[None for x in range(self.cols_count)] for y in
                      range(self.rows_count)]
        #short: 0, tall: 1
        #black: 0, white: 1
        #circle: 0, square: 1
        #solid: 0, hollow: 1
        self.pieces.append(Piece(0b0000))
        self.pieces.append(Piece(0b0001))
        self.pieces.append(Piece(0b0010))
        self.pieces.append(Piece(0b0011))
        self.pieces.append(Piece(0b0100))
        self.pieces.append(Piece(0b0101))
        self.pieces.append(Piece(0b0110))
        self.pieces.append(Piece(0b0111))
        self.pieces.append(Piece(0b1000))
        self.pieces.append(Piece(0b1001))
        self.pieces.append(Piece(0b1010))
        self.pieces.append(Piece(0b1011))
        self.pieces.append(Piece(0b1100))
        self.pieces.append(Piece(0b1110))
        self.pieces.append(Piece(0b1101))
        self.pieces.append(Piece(0b1111))

    def get_board(self):
        """
        Returns the board. 
        """
        return self.board

    def get_pieces(self):
        """
        Returns the pieces array.
        """
        return self.pieces

    def get_pieces_names(self):
        """
        Returns the names of all the pieces in our pieces array.
        """
        return ["(" + str(i) + "): " + self.pieces[i].get_piece_name()
                for i in range(0, len(self.pieces))]

    def get_rows(self):
        """
        Returns the rows of the board.
        """
        return self.board

    def get_cols(self):
        """
        Returns the columns of the board.
        """
        cols = []
        for row in self.board:
            cols.append([row[i] for i in range(self.rows_count)])
        return cols

    def shared_attributes(self, lst):
        """
        Returns a list of shared attributes among pieces.
        """
        attributes_list = [piece.get_attributes() for piece in lst
                           if piece] #nested list of all attributes
        if len(attributes_list) != self.rows_count:
            return 0
        win = 0b1111
        win2 = 0b1111
        for attr in attributes_list:
            win = win & attr
            win2 = win2 & ~attr
        return win or win2

    def check_win_horizontal(self):
        """
        Returns a boolean indicating whether or not a horizontal win has occurred.
        """
        for row in self.get_rows(): #0 to 3
            if self.shared_attributes(row): #if there are shared attributes
                return True
        return False

    def check_win_vertical(self):
        """
        Returns a boolean indicating whether or not a vertical win has occurred.
        """
        attr_list = [[] for i in range(self.rows_count)]
        for row in self.get_rows():
            for i in range(len(row)):
                attr_list[i].append(row[i]) #if there are shared attributes
        for lst in attr_list:
            if self.shared_attributes(lst):
                return True
        return False

    def check_win_diagonal(self):
        """
        Returns a boolean indicating whether or not a diagonal win has occurred.
        """
        ltr_diag = []
        rtl_diag = []
        i, j = 0, 3
        for row in self.board:
            ltr_diag += [row[i]]
            rtl_diag += [row[j]]
            i += 1
            j -= 1
        if (self.shared_attributes(ltr_diag) or
                self.shared_attributes(rtl_diag)):
            return True
        return False

    def player(self):
        """
        Returns whose turn it is.
        """
        if len(self.pieces)%2 == 0:
            return 1
        else:
            return 2

    def other_player(self):
        """ 
        Returns the other person's turn.
        """
        if len(self.pieces)%2 == 0:
            return 2
        else:
            return 1

    def print_board(self):
        """
        Prints the board.
        """
        for row in self.board:
            pr = []
            for piece in row:
                if piece:
                    pr.append(piece.get_piece_abbr())
                else:
                    pr.append(None)
            print(pr)

    def place_piece(self, piece, row, col):
        """
        Places the piece in the specified location.
        """
        if not self.board[row][col] and piece in self.pieces:
            self.board[row][col] = piece
            # del self.pieces[piece]
            self.pieces.remove(piece)
            return True
        else:
            return False

quarto_board = None

def initial_position():
    """
    Initializes the board, which is a 4x4 2D array.
    """
    return Board()

def primitive(state):
    """
    Returns whether or not the current game is a TIE, WIN, or UNDECIDED.
    This function takes in a state, which is an instance of Board.
    """
    if (state.check_win_horizontal() or state.check_win_vertical() or
            state.check_win_diagonal()):
        return WIN
    # no more pieces
    if len(state.pieces) == 0:
        return TIE
    return UNDECIDED

def gen_moves(state):
    """
    Generates a list of possible pieces for your opponent to give you.
    """
    return state.get_pieces_names()

def do_move(move, state):
    """
    Makes the move.
    """
    state.place_piece(move[0], move[1], move[2])
    return state

def solve(state):
    """
    Solver for our game instance.
    """
    return primitive(state)

def main():
    """
    The main function that keeps the game running.
    This is essentially an interface between our quarto game
    and the player.
    """
    print("Starting game of Quarto...")
    print()
    board = initial_position()
    while True:
        print("------------")
        print("Player", board.player(), "'s turn: ")
        print("------------")
        print("Current state of board: ")
        board.print_board()
        print()
        print("Available pieces: ")
        print(gen_moves(board))
        print()
        piece_index = input("Player " +str(board.other_player()) +
                            " please pick a piece to give to Player "
                            + str(board.player()) +
                            " (index num): ")
        p = board.get_pieces()[int(piece_index)]
        print("Player " + str(board.player()) +
              " choose where you want to place " + p.get_piece_name() + "...")
        r = input("Row: ")
        c = input("Col: ")
        do_move((p, int(r), int(c)), board)
        if solve(board) == WIN:
            board.print_board()
            print("Player", board.other_player(), "wins!")
            return
        else:
            print("SOLVE: ", solve(board))
        print()

main()
\$\endgroup\$
  • \$\begingroup\$ Please try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Nov 14 '16 at 10:58
  • 1
    \$\begingroup\$ What is Quarto? \$\endgroup\$ – Mathias Ettinger Nov 14 '16 at 11:06
  • \$\begingroup\$ Sorry about the vagueness! Quarto is a two-player 4x4 board game where you have 16 pieces, each of which has one of the following attributes: tall/short, red/blue, square/circular, hollow-top/solid-top (which I tried to represent with the bit strings). At each turn, you pick a piece for your opponent to place, and you win by placing a piece such that you get four pieces in a row that share some sort of attribute. \$\endgroup\$ – alvomwcc Nov 14 '16 at 18:44
  • \$\begingroup\$ I was curios about how it would turn out so I wrote my own version: codereview.stackexchange.com/questions/147026/… \$\endgroup\$ – Caridorc Nov 14 '16 at 21:27
4
\$\begingroup\$

You do not need getters, remove all of them

You can just access an attribute of a class directly: class.attribite_name. Using @property allows adding operations before retrieving the value. (See link above for more information).

any

When you want to verify that "at least one" condition is True, any is the best way to do it.

One example:

for lst in attr_list:
    if self.shared_attributes(lst):
        return True
return False

Becomes:

return any(self.shared_attributes(lst) for lst in attr_list)

Much shorter and more abstract.

Repetition

Some logic is shared between check_win_horizontal and check_win_vertical . I suggest factoring it out to a function to reduce repetition.

Columns

The columns can be obtained with zip(* board) so you can avoid writing the logic yourself.

Repetition

self.pieces.append(Piece(0b0000))
# ...... Many more similar lines

You can use a loop to make it clear that only the number changes and to avoid code bloat. Even better would be to generate this numbers programmatically to avoid the human error of forgetting/getting wrong one of them.

Confusing / weird aliases

def gen_moves(state): return state.get_pieces_names()
def do_move(move, state): state.place_piece(move[0], move[1], move[2]); return state
def solve(state): return primitive(state)

These functions are pretty much just aliases of other functions, and in my opinion they only produce code bloat (a lot of lines of code represent no logic at all, just other names for already stated logic).

Even more confusing is that you throw away the return value of do_move when you call it, so the return statement can easily be removed so the function becomes even more similar to place_piece

Just remove these and use the originals.

Good: separation of (input and output) from logic

This is the best feature of your code. Separating the user interaction from the logic makes everything simpler and neater.

\$\endgroup\$
  • \$\begingroup\$ I would add that the use of bit magic is very weird to see in Python, and I do not understand all those binary operators in your code. Probably a much simpler "higher level" solution exist, I just cannot tell it not understanding the operators. \$\endgroup\$ – Caridorc Nov 14 '16 at 14:53
  • \$\begingroup\$ good answer, however I think the bit operations are good, and common. For example re uses them too, e.g. to input multiline Unicode regexes I can use re.U | re.M which is 32 | 64 or 0b0100000 | 0b1000000. \$\endgroup\$ – Peilonrayz Nov 14 '16 at 15:09
  • \$\begingroup\$ @Peilonrayz interesting point \$\endgroup\$ – Caridorc Nov 14 '16 at 15:31
  • \$\begingroup\$ @Caridorc: there are different pieces in the game that differ by shape (circle or square), height (tall or short), color (black or white)... Presumably the bits represent the kind of piece. My first thought is an enum of some kind but bit operations have some advantages given the existing operators they are equipped with. \$\endgroup\$ – Dair Nov 14 '16 at 17:38

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.