6
\$\begingroup\$

I'm a beginner-intermediate programmer who's mostly used Java. I taught myself Python and made this engine for playing Reversi. Tips on Pythonic ways to accomplish tasks and/or general advice appreciated!

from ast import literal_eval as eval

board_size = 8
BLACK = '\u26AB'
WHITE = '\u26AA'
EMPTY = '\u2B1c'
offsets = ((0,1),(1,1),(1,0),(1,-1),(0,-1),(-1,-1),(-1,0),(-1,1))

def inverse(piece):
    return BLACK if piece is WHITE else WHITE

def main():
    if input('Display instructions? [Y/N]: ').lower() == 'y':
        instructions()
    board = create_board()
    piece = BLACK
    while has_valid_move(board, piece):
        game_loop(board, piece)
        if has_valid_move(board, inverse(piece)):
            piece = inverse(piece)
    print_board(board)
    black, white = 0,0
    for row in board:
        for token in row:
            if token is WHITE: white += 1
            if token is BLACK: black += 1
    if black == white:
        print("It's a tie!")
    else:
        print()
        print('{token} is the winner!' % (BLACK if black>white else WHITE))
    return

def instructions():
    print('''
Reversi, also known as Othello, is a strategy board game for two players.
The two players alternate placing Black and White tokens on the board,
respectively, and the winner is whomever has the most tokens of their color
on the board when no legal moves remain.'''\
          )
    input('\nPress Enter to continue...')
    print('''
A legal move is one that causes other pieces to flip between White and Black.
A move causes pieces of the opposite color to flip when placed down if and
only if there is an unbroken line between it and another piece of the same
color consisting of pieces of the opposite color.'''\
          )
    input('\nPress Enter to continue...')
    print('''
To play this version, when prompted, enter the cartesian coordinates of the
location that you would like to place the piece. The upper left corner of the
board is 0,0 and the lower right is {0},{0}.'''.format(board_size-1)\
          )
    input('\nPress Enter to continue...')

def create_board():
    board = [[EMPTY for x in range(board_size)] for x in range(board_size)]
    half = board_size//2
    board[half-1][half-1] = WHITE
    board[half][half] = WHITE
    board[half-1][half] = BLACK
    board[half][half-1] = BLACK
    return board

def print_board(board):
    for row in range(len(board)):
        print(*board[row], sep='')
    return

def game_loop(board, piece):
    print()
    print_board(board)
    while(True):
        try:
            move = eval(input('Place %s where? ' % piece))
            move = tuple(reversed(move))
            # x,y -> y,x (easier to use)
            if is_valid_move(board, piece, move):
                place_piece(board, piece, move)
                return
            else:
                raise AssertionError
        except (TypeError, ValueError, IndexError, SyntaxError, AssertionError):
            #   ------------------bad  input------------------  ---bad move---
            print('Invalid move. Try again.')

def is_valid_move(board, piece, move):
    if board[move[0]][move[1]] is not EMPTY: return False
    for offset in offsets:
        check = [move[0]+offset[0], move[1]+offset[1]]
        while 0<=check[0]<board_size-1 and 0<=check[1]<board_size-1 and \
              board[check[0]][check[1]] is inverse(piece):
            check[0] += offset[0]
            check[1] += offset[1]
            if board[check[0]][check[1]] is piece:
                return True
    return False

def place_piece(board, piece, move):
    board[move[0]][move[1]] = piece
    for offset in offsets:
        check = [move[0]+offset[0], move[1]+offset[1]]
        while 0<=check[0]<board_size and 0<=check[1]<board_size:
            if board[check[0]][check[1]] is EMPTY: break
            if board[check[0]][check[1]] is piece:
                flip(board, piece, move, offset)
                break
            check[0] += offset[0]
            check[1] += offset[1]
    return

def flip(board, piece, move, offset):
    check = [move[0]+offset[0], move[1]+offset[1]]
    while(board[check[0]][check[1]] is inverse(piece)):
        board[check[0]][check[1]] = piece
        check[0] += offset[0]
        check[1] += offset[1]
    return

def has_valid_move(board, piece):
    for y in range(board_size):
        for x in range(board_size):
            if is_valid_move(board, piece, (y,x)): return True
    return False

if __name__ == '__main__':
    main()

There aren't that many comments as the code should be rather self-documenting.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome to Code Review! I hope you get some good answers. \$\endgroup\$
    – Phrancis
    Apr 10, 2016 at 19:16

2 Answers 2

2
\$\begingroup\$
from ast import literal_eval as eval

It's fabulous that you are using ast.literal_eval() instead of eval(). You shouldn't be naming it eval, however, because you are shadowing the built-in function. If sometime later you decide that you want to use eval() for some reason, you might be surprised if it doesn't work how you expect it to.

board_size = 8
BLACK = '\u26AB'
WHITE = '\u26AA'
EMPTY = '\u2B1c'
offsets = ((0,1),(1,1),(1,0),(1,-1),(0,-1),(-1,-1),(-1,0),(-1,1))

You use constants instead of magic values! board_size and offsets are both constants and should be ALLCAPS, but you aren't using magic values!

def main():

That's good. Your main code is in a function instead of at module level.

if input('Display instructions? [Y/N]: ').lower() == 'y':

What if the user typed yes? It would accept it, but treat it as no. You should validate the input before continuing. You might want to create a function that keeps asking until y or n is given. You might get more fancy and go with yes, yeah, uh huh, yeah, man!, ... whatever you want to do.

print('{token} is the winner!' % (BLACK if black>white else WHITE))

It looks like you're a little confused about what you're gonna do. That throws a TypeError. Either use '{token} ...'.format(token=...) or '%s...' % ..., but don't try to merge them. I prefer .format(), but you can do what you want.

return

It's useless. Take it out. You're already at the end of a function that doesn't return anything above, so it will return None implicitly. There is no need to add the extra line.

...'''\
    )

Unclosed parentheses are also line-continuers. You don't need the backslash. You can remove all of those backslashes in that function. By the way, I'm very pleased to see that you aren't using magic values. You use board_size instead of writing in the actual numbers.

board = [[EMPTY for x in range(board_size)] for x in range(board_size)]

It's great that you are using list comprehensions. I would use _ instead of x just to make it a little more obvious that it isn't being used.

def print_board(board):
    for row in range(len(board)):
        print(*board[row], sep='')
    return

You aren't using row for anything except as an index for board. You should just iterate through board in the first place:

for row in board:
    print(*row, sep='')

My second problem is again the return.

while(True):

The parentheses are useless. Just do while True: You can change that in several places.


I would highly recommend classes as pointed out by @kushj. Even if you changed just that, your code would look very good.

\$\endgroup\$
1
  • \$\begingroup\$ Those last two are my Java heritage showing through :P Thank you for this wonderful insight, though; it was very useful! \$\endgroup\$
    – CAD97
    Apr 11, 2016 at 23:38
2
\$\begingroup\$

Here are my quick impressions. Please note that I haven't gone through your code line by line, and I hope someone would offer a detailed review for you soon.

Let's look at function:

def is_valid_move(board, piece, move):
    if board[move[0]][move[1]] is not EMPTY: return False
    for offset in offsets:
        check = [move[0]+offset[0], move[1]+offset[1]]
        while 0<=check[0]<board_size-1 and 0<=check[1]<board_size-1 and \
              board[check[0]][check[1]] is inverse(piece):
            check[0] += offset[0]
            check[1] += offset[1]
            if board[check[0]][check[1]] is piece:
                return True
    return False

On the fly, there are some issues which could be seen.

To understand the code, I would need to understand the structure of board, move and piece. And to do that, I would need to go back in stack, and see their composition.

Here are some ideas which could alleviate this:

  1. Use of descriptive data structures rather than simple 2-D arrays. Instead of saying move[0], move[1], move.x and move.y is much more clearer. Namedtuple is one such structure.
  2. Document, document, and document. You mentioned that you would rather have code self-explanatory. That is good and excellent aim. However, as program size would grow (and I hope you would start writing programs which are non-trivial in size) and more importantly, as number of people associated with project grows, importance of documentation grows. When someone else wants to use your function (and six months down the line, you also become "someone else"), you don't want them to peruse other functions to find out what exactly your function does, in what format does it accept argument, and what is it return format. Check out what docstrings are, and use them. Extensively.
  3. This brings me to third point. Use classes. Since you don't want to explain to everyone again and again what is board, and how to represent move,plus it makes logical sense, encapsulate related functions and data structures in one entity. Using classes IMO would have been lot cleaner.

I would love to come back to this question later, and offer a detailed review on other metrics.

Here is some general Python advise:

  1. PEP 8. If you have just started Python, this is must read.
  2. Avoid global constants. (Here, if you would have used classes, this would be remedied)
  3. I noticed you were using return in several functions when it was not strictly necessary.

Caught my eye: Instead of if is_valid_move(board, piece, (y,x)): return True can use return is_valid_move(board, piece, (y,x))

\$\endgroup\$
3
  • \$\begingroup\$ Can't use return is_valid_move(board, piece, (y,x)). If is_valid_move() returns False, he wants to continue, not return False. \$\endgroup\$
    – zondo
    Apr 11, 2016 at 22:26
  • \$\begingroup\$ Thank you for your input! Especially the suggestion to use namedtuple was something that I missed in my own research. I've accepted @zondo's answer as it covers a larger review, but you still have my +1 for this answer. \$\endgroup\$
    – CAD97
    Apr 11, 2016 at 23:39
  • \$\begingroup\$ And just as an aside, I have one non-trivial project I'm working on in Java. It's been going for a while, so I know the "why did I do this" very well. I'll probably never do large-scale work in Python because I prefer strongly-typed languages, but who knows, for we are always learning :) \$\endgroup\$
    – CAD97
    Apr 11, 2016 at 23:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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