patternpythonMinor
First Chess Project (ver 1.1)
Viewed 0 times
chessprojectfirstver
Problem
I'd already asked for my code to be reviewed earlier here
First Chess project)
Full code
Considering the suggestions put forth for that question, I spent a day and refactored my whole code. For those who have seen my previous question/code, could you you point out anything which I have as of yet not improved upon?
For everyone else, all I want to know is whether I am on the right track while programming in python. Could you please tell me if there is something unpythonic in my code? What parts are not written in the way "good Python code" is meant to be written? Here is a snippet I feel is not really good:
This is in the main loop:
```
input_read=False
if len(m)==4:
X1,Y1,X2,Y2 = m # assigning final co-ordinates
m=[] # empty the buffer input list
input_read=True
if not input_read:
events=pygame.event.get()
for event in events:
if event.type == pygame.QUIT: # Quitting
exit(0)
if event.type == pygame.MOUSEBUTTONUP: pressed= False # get ready for next click
if event.type == pygame.MOUSEBUTTONDOWN and not pressed:
pressed=True
if event.button==1:
if len(m)==2: # add second click co-ordinates
pos = pygame.mouse.get_pos()
x = pos[0]/80+1
y = pos[1]/80+1
m.extend([x,y])
else:
pos = pygame.mouse.get_pos() # add first click co-ordinates
x = pos[0]/80+1
y = pos[1]/80+1
j=checkPositionToPieceDict((x,y)) # Checks that the first click has a piece
if j: # of your colour and selects it.
First Chess project)
Full code
Considering the suggestions put forth for that question, I spent a day and refactored my whole code. For those who have seen my previous question/code, could you you point out anything which I have as of yet not improved upon?
For everyone else, all I want to know is whether I am on the right track while programming in python. Could you please tell me if there is something unpythonic in my code? What parts are not written in the way "good Python code" is meant to be written? Here is a snippet I feel is not really good:
- Taking input from user
This is in the main loop:
```
input_read=False
if len(m)==4:
X1,Y1,X2,Y2 = m # assigning final co-ordinates
m=[] # empty the buffer input list
input_read=True
if not input_read:
events=pygame.event.get()
for event in events:
if event.type == pygame.QUIT: # Quitting
exit(0)
if event.type == pygame.MOUSEBUTTONUP: pressed= False # get ready for next click
if event.type == pygame.MOUSEBUTTONDOWN and not pressed:
pressed=True
if event.button==1:
if len(m)==2: # add second click co-ordinates
pos = pygame.mouse.get_pos()
x = pos[0]/80+1
y = pos[1]/80+1
m.extend([x,y])
else:
pos = pygame.mouse.get_pos() # add first click co-ordinates
x = pos[0]/80+1
y = pos[1]/80+1
j=checkPositionToPieceDict((x,y)) # Checks that the first click has a piece
if j: # of your colour and selects it.
Solution
This code seems much better than before, so congrats. Here are a few more things to adjust:
-
Move your main game loop into a function and call it in an if block like this:
This is so that people (including yourself) importing your module don't immediately get a chess window popping up.
-
You factored out the
-
At this point, with the number of global variables and global functions operating on those variables, you should group them into a
-
I will second the recommendations to replace numbers that are clearly constants with variables to improve readability, as well as follow the style guidelines in PEP 8.
Beyond that, I think the changes we're looking at are either implementation specific (you would do it differently depending on what this is supposed to accomplish, and what your goals are), are too minor to bring up one by one, or down to personal coding style.
-
Move your main game loop into a function and call it in an if block like this:
if __name__ = "__main__":
#call your main functionThis is so that people (including yourself) importing your module don't immediately get a chess window popping up.
-
You factored out the
Piece baseclass from all of the pieces, simplifying that code quite a bit. This is good! But now you should finish the job of merging the different coloured pieces into the same class. They are extremely close to identical. You can pass in all of the values that distinguish them (colour, position, etc.) in the __init__ method when you initialize them.-
At this point, with the number of global variables and global functions operating on those variables, you should group them into a
GameState object to encapsulate them.-
I will second the recommendations to replace numbers that are clearly constants with variables to improve readability, as well as follow the style guidelines in PEP 8.
Beyond that, I think the changes we're looking at are either implementation specific (you would do it differently depending on what this is supposed to accomplish, and what your goals are), are too minor to bring up one by one, or down to personal coding style.
Code Snippets
if __name__ = "__main__":
#call your main functionContext
StackExchange Code Review Q#43262, answer score: 2
Revisions (0)
No revisions yet.