First Chess project

I'd made a project using pygame in python around 3 months ago. This was my first big project after I started learning how to program in college.

Now I'd like to know just how good my programming syntax and style is. Whether there are any weird irritants that simply defy the unwritten rules of programming in python. I'd like to know them so that I can nip the evil in the bud.

Could someone please go through the code, and tell me where somethings have been implemented as they should have been and where something else might work better... Or if in general a better (read 'more pythonic'- I've never really understood what people mean when they say that btw.), style would work and what is it.

As of now, the chess works. The only thing is you gotta control both the black and white pieces by yourself.

Please just review the code.

Full code source

For the parts I think might not be implemented the way they could be:

1) Implementing a timer as:

import time
time_change_var=time.asctime().split()[3].split(":")[2]
time_elapsed=0

then in the main loop of game cycles:

time_change_check=time.asctime().split()[3].split(":")[2]                               
if time_change_var!=time_change_check:
    time_elapsed+=1
    time_change_var=time_change_check
time_elapsed_print=font.render("%d:%d"%(time_elapsed/60,time_elapsed%60),True,white)
screen.blit(time_elapsed_print,[730,600])
screen.blit(font.render("Time Elapsed",True,black),[660,565])

2) Taking 2 consecutive clicks from the user that are valid

A click is valid when:
(i). First click is on a box containing a piece
(ii). Second click is on a box containing either a piece of other colour or is empty
I am doing it like this:

```
input_read=0
if len(m)==4:
X1=m[0] #assigning final inputs
Y1=m[1]
X2=m[2]
Y2=m[3]
m=[] # empty the buffer input list
input_read=1

if not input_read:
event=pygame.event.wai

I spent some time last night reviewing this code. Here are some specific suggestions I can make:

-
Move your game loop into an if __name__= "__main__": statement at the bottom of the code. Currently, if the module is imported, the person who imports it will immediately start playing chess, which isn't desirable (they may just want to know about the piece classes.

-
Group variables which go together in classes. By "go together", I mean that they share a common purpose, like score counting. It will help keep track of where those variables are used.

-
Use parameters on class init methods to reduce duplication. There is no reason to have a BlackPawn class and a WhitePawn class, when they're only different by a few characters. The only difference between a white pawn and a black pawn is the colour and direction, so pass those in as parameters and have one Pawn class.

-
Group common functionality into functions. The queen and rook both move and capture horizontally, so that code only needs to be written once and can be shared between both classes. The queen and bishop both move diagonally, so that functionality can be shared between both classes. All of the pieces are checking for boundary conditions, so that functionality can be shared among all of them. Breaking your functions up in this way will really help readability, since instead of 1

-
Reduce and combine duplicate code in general. If two pieces of code are identical except for one variable, extract that part of the code as a method and pass the variable in. The less code you have, the more readable and maintainable it is. Looking at the queen class, there are 200 lines of code that could easily be reduced to 50 or even less just by breaking down each section (the contents of if statements are usually a good place to extract a method from).

-
Reduce or remove magic values. In python it's better to use strings ('black' and 'white') rather than integers to signify properties. That will make the code more self documenting. if colour == 'white': is a lot clearer than 'if colour == 1:' .People reading your code shouldn't need to look around for the definitions of your magic numbers.

-
I found this: "if it is possible to kill the other opponent. My function automatically kills the opponent while checking for valid moves. Hence I had to make the kill_count function to ensure that the opponent stays alive even when his box is checked for a possible move." This is the strongest argument that "check" and "kill" should be split into separate methods. You mentioned that you wanted to add AI? Then you will definitely want to do that, or else you will kill every pieces the AI thinks it might be able to capture. In general, split up methods that do more than one thing (check AND kill) unless you're completely sure that you'll never want to do one without doing the other. As soon as you're thinking "man, it sure is inconvenient that other thing happens when I do thing, split thing and other thing into separate methods to relieve the tension.

-
except ZeroDivisionError: pass` Never never let errors pass silently. Instead, fix the bug which is causing the error. Trust me, having debugged codebases that do this a lot, this is the surest road to insanity there is.

-
Reduce the number of global variables in general. Ideally, global variables should be constants, if they exist at all. Too many global variables can make a module difficult to debug, since it's not easy to see when each variable is being modified. This will be easier if you follow 1 and group your variables into classes.

StackExchange Code Review Q#42514, answer score: 15