Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

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

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

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

added 17 characters in body
Source Link
zondo
  • 3.9k
  • 14
  • 27

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 builbuilt-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.

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

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

Not so yay. WhatWhat 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.

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 buil-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.

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

Yay! Code is in a function instead of at module level!

Not so yay. 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.

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.

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!

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

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.

Source Link
zondo
  • 3.9k
  • 14
  • 27
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 buil-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.

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))

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():

Yay! Code is in a function instead of at module level!

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

Not so yay. 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.

lang-py

AltStyle によって変換されたページ (->オリジナル) /