1
\$\begingroup\$

I wrote a simple tic tac toe game in python. (I wrote the game logic and implementation separately) (in this case, it's a command line interface, I may do a Tkinter version) Can you review the code and give me tips for the future ?

logic.py

from itertools import product

TIC = 0
TAC = 1
BLANK = -1
WINNERS = (
    ((0, 0), (0, 1), (0, 2)),
    ((1, 0), (1, 1), (1, 2)),
    ((2, 0), (2, 1), (2, 2)),
    ((0, 0), (1, 0), (2, 0)),
    ((0, 1), (1, 1), (2, 1)),
    ((0, 2), (1, 2), (2, 2)),
    ((0, 0), (1, 1), (2, 2)),
    ((2, 0), (1, 1), (0, 2)),
)
INDICES = list(product(range(3), range(3)))

# keep track of the game using a dictionary
# keys are the indices (i, j) where 0 <= i, j <=2
# where i represents the row & j the column

def init_state():
    """returns a blank state dictionary"""
    state = dict()
    for tup in INDICES:
        state[tup] = BLANK
    return state

def is_valid_move(state, tup):
    """returns a boolean, true if the move is valid"""
    return state[tup] == BLANK

def tic_turn(state, tup):
    """[TIC] assumes the move is valid, and returns a new state
    dictionary with updated values"""
    state = dict(state)
    state[tup] = TIC
    return state


def tac_turn(state, tup):
    """[TAC] assumes the move is valid, and returns a new state
    dictionary with updated values"""
    state = dict(state)
    state[tup] = TAC
    return state

def is_won(state):
    """returns a boolean, true if the game is won"""
    for winner in WINNERS:
        vals = set(state[tup] for tup in winner)
        if len(vals) == 1 and BLANK not in vals:
            return True
    return False

def state_generator(get_input):
    """yields successive states
    takes a function with optional string argument
    that returns a tuple of numbers in range(3)"""
    state = init_state()
    yield state
    for level in range(5):

        human_input = get_input('TIC')
        machine_input = human_input[0] - 1, human_input[1] - 1
        state = tic_turn(state, machine_input)
        yield state
        if is_won(state):
            print("tic won!")
            break

        if level == 4:
            print("Tie!")
            break

        human_input = get_input('TAC')
        machine_input = human_input[0] - 1, human_input[1] - 1
        state = tac_turn(state, machine_input)
        yield state
        if is_won(state):
            print("tac won!")
            break

main.py

from logic import TIC, TAC, BLANK, INDICES, state_generator

def get_int(info, kind):
    """prompts the user with prompt=info
    for a numerical value of type=kind
    return integer or quits program"""
    try:
        data = input("\t\t[%s] %-6s: " % (info, kind))
        data = data.strip()
        if data.capitalize() == 'Q':
            exit()
        data = int(data)
        if data not in range(1, 4):
            raise Exception()
        return data
    except Exception as e:
        print("\t\t[%s] Invalid input." % info)
        print("\t\t[%s] Try again\n" % info)
        return get_int(info, kind)


def get_input(info=""):
    """return tuple of numbers in range: 1,2,3 from user input"""
    return get_int(info, "Row"), get_int(info, "Column")


def display_grid():
    """creates display grid where to show the game steps"""
    ws = 5 * " "
    lm = 5 * "_"
    ds = "  %s  "
    cl_line = 2 * "\t" + " " + 17 * "_" + " " + "\n"
    ws_line = 2 * "\t" + "|" + ws + "|" + ws + "|" + ws + "|" + "\n"
    lm_line = 2 * "\t" + "|" + lm + "|" + lm + "|" + lm + "|" + "\n"
    ds_line = 2 * "\t" + "|" + ds + "|" + ds + "|" + ds + "|" + "\n"
    bloc = ws_line + ds_line + lm_line
    return "\n" + cl_line + 3 * bloc

def draw(state):
    """draws the state of the game using a display_grid"""
    to_str = {TIC: 'X', TAC: 'O', BLANK: ' '}
    grid = display_grid()
    _list = []
    for tup in INDICES:
        _list.append(to_str[state[tup]])
    params = tuple(_list)
    print(grid % params)

print("\n\n\t\t My Tic Tac Toe game")
print("\n\n\t\t  Press q to exit.")
for state in state_generator(get_input):
    draw(state)

Snapshot enter image description here

\$\endgroup\$
1
\$\begingroup\$

I would create a TTTBoard class with the logic to draw it as text, fill positions and check if the game is won as methods of it. Then the main program would create an instance and process input like it already does but not worry about much else.

\$\endgroup\$
1
\$\begingroup\$

is_valid_move(state, tup): Is the given move really valid? If the tuple (123,4.71) is given, is it a valid move, invalid move, or a crash-the-program input? It can only be valid if the tuple exists as a key in the state, so perhaps:

return tup in state  and  state[tup] == BLANK

tic_turn(state, tup) and tac_turn(state, tup) share almost all the same code. The only difference (other than the docstring) is the value assigned to state[tup]. You could make one function, and pass in the player as an argument.


Both tic_turn and tac_turn take the current state of the board, duplicate the state dictionary, modify the copy to produce a new state, and return that new state. Why? If you are coming from a functional programming paradigm, where mutable objects are eschewed, I suppose I could forgive you. If you are planning on implementing an AI which has to search the game state many moves in the future to find the best move, I suppose I could forgive you. But with what is written so far, it would be far, far simpler to simply modify the current state; then you could return something else ... like whether the move was a winning move or not. Speaking of which ...


is_won(state):. You are looping over all triplets of possible WINNERS, forming a set from the contents of the state for each tuple in the triplet, and returning True if there is only one unique value in the set ... as long as that value wasn't BLANK. Doesn't that seem overly complicated? I think so. After TIC moves, we don't care about the possibility of a win by TAC. After TAC moves, we don't care about the possibility of a win by TIC. You could pass in the player that just made a move and look for a triplet composed of only that player, which is far simpler to express in code:

def is_won(state, player):
    for winner in WINNERS:
        if all(state[tup] == player for tup in winner):
            return True
    return False

or

def is_won(state, player):
    return any(all(state[tup] == player for tup in winner) for winner in WINNERS)

What is this human_input verses machine_input all about? The computer is not producing input for the game as if it were a player. Okay, I understand that you are converting human-centric counting of row/column numbers from 1-3 into 0-2, but why are you using 0-2? You create tuples for all board locations, and use these generated tuples through-out the code. Why not use the "human friendly" tuples values as the keys, and ditch your "machine" versions, like:

WINNERS = (
    ((1,1), (1,2), (1,3)),
       ...
    ((3,1), (2,2), (1,3)),
)

state_generator(get_input): Oh, where do I start? First, you've got duplicated code for TIC and for TAC. If you had a player variable, you could start with player = TIC, and toggle between players with player = 1 - player. Then, you could loop over the 9 possible turns, instead of over the 5 pairs of turns. The ugly hard-coded 5th turn-pair special case of a "tie" goes away; if the loop exits after all 9 moves and no winner, then you have a tie.

Hard coded mysterious numbers like 4 and 5? Where did they come from? No comment in sight which explains it. Slightly better is the above suggestion of looping over 9 turns. But, again, why use 9? How about len(INDICES)? Or better, loop while there are still valid moves someone can make. Then you can experiment with 4x4 tic-tac-toe grids or other variants:

player = TIC

while BLANK in state.values():
    # get input, update state, yield state, break if won
    player = 1 - player
else:
    print("Tie!")

Finally, using a generator construct, with input dependency injection, which blocks waiting for input from the console? Yes, it works, but don't do this. You could easily replace this with a function with a simple loop, and pass both get_input and draw as arguments to the function.


get_int(...): You import all sorts of values from logic, but you are checking for data not in range(1,4). Why is this hard-coded? What if you wanted a 4x4 tic-tac-toe game? You could define VALID_ROWS and VALID_COLS as range(1,4) in "logic" and import those values, and pass the valid range as an argument to get_int.


get_input():. Why does it have a default for info? A value is always explicitly being given; the default just adds noise to the code. Interestingly, the the user input is never checked to see if the is_valid_move test passes. You can put an X on top of your opponent's O...


draw(state): Whenever you see tmp = [] followed by for x in y: wrapping a tmp.append(...), you can almost always replace this with list comprehension, eliminating the costly list.append() operations:

params = tuple(to_str[state[tup]] for tup in INDICES)
\$\endgroup\$
  • \$\begingroup\$ Hello, thank you for the feedback! Some of the things you evoked I did on purpose (like creating a state variable each turn), others I just forgot about or are actually bad design. I'll definitely re-write and refactor the code. Thanks again for the help! \$\endgroup\$ – aripy887 Nov 7 '18 at 15:25

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.