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.
-
2\$\begingroup\$ Welcome to Code Review! I hope you get some good answers. \$\endgroup\$Phrancis– Phrancis2016年04月10日 19:16:44 +00:00Commented Apr 10, 2016 at 19:16
2 Answers 2
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.
-
\$\begingroup\$ Those last two are my Java heritage showing through :P Thank you for this wonderful insight, though; it was very useful! \$\endgroup\$CAD97– CAD972016年04月11日 23:38:28 +00:00Commented Apr 11, 2016 at 23:38
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:
- Use of descriptive data structures rather than simple 2-D arrays. Instead of saying
move[0]
,move[1]
,move.x
andmove.y
is much more clearer. Namedtuple is one such structure. - 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.
- 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:
- PEP 8. If you have just started Python, this is must read.
- Avoid global constants. (Here, if you would have used classes, this would be remedied)
- 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))
-
\$\begingroup\$ Can't use
return is_valid_move(board, piece, (y,x))
. Ifis_valid_move()
returnsFalse
, he wants to continue, not returnFalse
. \$\endgroup\$zondo– zondo2016年04月11日 22:26:43 +00:00Commented 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– CAD972016年04月11日 23:39:57 +00:00Commented 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– CAD972016年04月11日 23:44:41 +00:00Commented Apr 11, 2016 at 23:44