2
\$\begingroup\$

What can be improved?

#!/usr/bin/python
import random

def board():
    board_size = int(raw_input("Board size: "))
    board = []
    for i in range(0,board_size):
        board.append(list(range(board_size)))
    for parent in board:
        for child in parent:
            parent[child] = 0
    return board

def checkHorizontal(table,token):
    # HORIZONTAL
    for row in table:
        length = len(row)
        check_row = []
        for column in row:
            if column == token:
                check_row.append(token)
        if len(check_row) == length:
            return True

def checkVertical(table,token):
        # VERTICAL
        for column in zip(*table):
            check_col = []
            for item in column:
                if item == token:
                    check_col.append(token)
                if len(check_col) == len(table[0]):
                    return True

def checkDiagonal(table,player_no):
    if all([table[i][i]==player_no for i in range(len(table)) ]):
        return True
    if all([table[-i-1][i]==player_no for i in range(len(table))]):
        return True
    return False

def whoWon(who, table, token):
    if who == 'x':
        if(checkHorizontal(table,token)): return True
        elif(checkVertical(table,token)): return True
        elif(checkDiagonal(table,token)): return True
    else:
        if(checkHorizontal(table,token)): return True
        elif(checkVertical(table,token)): return True
        elif(checkDiagonal(table,token)): return True

def placeToken(token,table,user_inp,position_taken_msg):
    if(table[user_inp[0]][user_inp[1]] == 0):
        table[user_inp[0]][user_inp[1]] = token
    else:
        while(table[user_inp[0]][user_inp[1]] != 0):
            print position_taken_msg
            user_inp = [0,0,0]
            user_inp[0] = int(raw_input('Row(0 - 2): '))
            user_inp[1] = int(raw_input('Col(0 - 2): '))
            if(table[user_inp[0]][user_inp[1]] == 0):
                table[user_inp[0]][user_inp[1]] = token
                break


def gamePlay():
    table, game_over, x_turn, o_turn, position_taken_msg, x_token, o_token = board(), False, True, False, "Position already taken! Choose again!.", 1, 2
    while not game_over:
        if x_turn:
            print '\n','-'*len('gandalf\'s'),'Gandalf\'s Turn!'
        else:
            print '\n','-'*len('saruman\'s'),'Saruman\'s Turn!'
        user_inp, user_inp[0], user_inp[1] = [0,0,0], int(raw_input('Row(0 - 2): ')), int(raw_input('Col(0 - 2): '))

        if x_turn:
            placeToken(x_token,table,user_inp,position_taken_msg)
            x_turn, o_turn = False, True
            print '\n'.join([str(p) for p in table])
            if(whoWon('x',table, x_token)):
                print "\n\n\t##########\n\tGandalf Won!\n\t##########\n\n"
                game_over = True
        else:
            placeToken(o_token,table,user_inp,position_taken_msg)
            x_turn, o_turn = True, False
            print '\n'.join([str(p) for p in table])
            if(whoWon('o',table, o_token)):
                print "\n\n\t##########\n\tSaruman Won!\n\t##########\n\n"
                game_over = True

def main():
    gamePlay()


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

Coding style

There's a limit to how much untidiness a reviewer is willing to tolerate. This code is right on the border. Please read PEP8 and follow the recommendations carefully. There is also a command line utility called pep8 that can check your program and tell you what to fix. There are also editors like PyCharm that warn you about violations of coding conventions, and help you fix them.

In my review below, I will try to not repeat what PEP8 can already tell about your code.

Return values

Many of the functions don't have an explicit return value. Take for example this:

def checkVertical(table,token):
        # VERTICAL
        for column in zip(*table):
            check_col = []
            for item in column:
                if item == token:
                    check_col.append(token)
                if len(check_col) == len(table[0]):
                    return True

The nested loop might be exited with a return True. If that doesn't happen, there's no other return statement, so the method will exit with None. This may work OK, as None is a falsy value in Python, but it would be cleaner to add a return False at the end to make this a proper boolean function.

Strange logic

What is going on in this function?

def whoWon(who, table, token):
    if who == 'x':
        if(checkHorizontal(table,token)): return True
        elif(checkVertical(table,token)): return True
        elif(checkDiagonal(table,token)): return True
    else:
        if(checkHorizontal(table,token)): return True
        elif(checkVertical(table,token)): return True
        elif(checkDiagonal(table,token)): return True

Regardless of the value of who, the same code gets executed. And again, there's no explicit return statement in case none of the conditions match. As it is, the function could be replaced with:

def whoWon(who, table, token):
    return checkHorizontal(table, token) or checkVertical(table, token) or checkDiagonal(table, token)

Separation of concerns

The board() function reads the board size and initializes a board. That's two distinct responsibilities. It would be better to split these to different functions.

Simplifications

The initialization of the board in the board() function can be simplified:

return [[0] * board_size for _ in range(board_size)]

checkHorizontal() can be similarly simplified:

def checkHorizontal(table, token):
    for row in table:
        if all(column == token for column in row):
            return True
    return False
\$\endgroup\$

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.