First Chess project

Question

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.wait()                   
    if event.type == pygame.QUIT:               
        done = True                             
    if event.type ==  pygame.MOUSEBUTTONDOWN:   
        if len(m)==2:
            if event.button==1:
                x1,y1=-1,-1
                pos = pygame.mouse.get_pos()    
                x2 = pos[0]/80+1                                            
                y2 = pos[1]/80+1
                m += [x2] + [y2]
                moves=[]

        else:
            if event.button==1:
                pos = pygame.mouse.get_pos()
                x1 = pos[0]/80+1
                y1 = pos[1]/80+1
                for elements in pieces_in_play:
                    if x1 == elements.x and y1 == elements.y:
                        if elements.colour==col:
                            f=elements
                            m += [x1] + [y1]

---

3) Building classes for the different pieces

I have built a class for each of the different type of piece. The class contains the object's name, colour,type,survival,x,y positions and the corresponding function which checks whether its possible to move the piece according to the input.It also kills the opponents piece if its already there on the destination box.

class BlackBishop(object):      
    n=1
    def __init__(self):
        self.name=int("2"+str(BlackBishop.n)+"0")
        self.colour = 0
        self.type = 2
        self.survive = 1
        self.y=8
        if BlackBishop.n==1:    
            self.x=3
        else:
            self.x=6
        BlackBishop.n += 1
        self.image = pygame.image.load("./images/bishop_b.png")

    def check(self,X1,Y1,X2,Y2):

        if (1<=X1<=8 and 1<=Y1<=8 and 1<=X2<=8 and 1<=Y2<=8):
            if abs(Y2-Y1)==abs(X2-X1):
                m=(Y2-Y1)/(X2-X1)
                q=[]
                t=[]
                if X2>X1:
                    x=X1
                    if Y2>Y1:
                        y=Y1
                        q.append((x,y))
                        while (x!=X2 and y!=Y2)and(x>0 and y>0):
                            x+=1
                            y+=1
                            q.append((x,y))

                    else:
                        y=Y1
                        q.append((x,y))
                        while (x!=X2 and y!=Y2)and(x>0 and y>0):
                            x+=1
                            y-=1
                            q.append((x,y))

                else:
                    x=X1
                    if Y2>Y1:
                        y=Y1
                        q.append((x,y))
                            while (x!=X2 and y!=Y2)and(x>0 and y>0):
                            x-=1
                            y+=1
                            q.append((x,y))

                    else:
                        y=Y1
                        q.append((x,y))
                        while(x!=X2 and y!=Y2)and(x>0 and y>0):
                            x-=1
                            y-=1
                            q.append((x,y))

                for coordinate in q:
                    for check_piece in pieces_in_play:
                        if coordinate==(check_piece.x,check_piece.y):
                            if check_piece!=self:
                                if check_piece.colour==0:
                                    return 0
                                elif check_piece.colour==1 and coordinate==(X2,Y2):
                                    check_piece.survive=0
                                    killcount(check_piece)
                                    return 1
                                else:    
                                    return 0
                else:
                    return 1
            else:
                return 0
        else:
            return -1

Answer 1

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

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

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

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

4. 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<=X1<=8 and 1<=Y1<=8 and 1<=X2<=8 and 1<=Y2<=8, you will have coord_is_within_bounds(), which says exactly what it does and what it's used for.

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

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

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

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

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

Answer 2

One thought ... Instead of having all those else blocks that only have a return statement, switch your if statements around, so that the if block only has a return statement, and then there doesn't need to be an else block for it. E.g., instead of ...

if 1<=X1<=8:
    original_contents_of_if_block
else:
    return -1

write ...

if not(1<=X1<=8):
    return -1
original_contents_of_if_block

Also, when you have several loops that do the same thing, you can convert them to a single function that gets called wherever needed.

Answer 3

Pythonic

Basically, a code is pythonic when it uses common Python idioms. In Python, there is usually one (and only one) way to do things, so you should try to know what this thing is, and use it. You won't fight against the language, the code will be easier to read and modify by other programmers.

A good way to start writing pythonic code is to follow PEP 8. It is a standard for Python indentation. If you follow PEP 8, your code will look more pythonic, and will be easier to read by fellow Python programmers. Of course, it's only the tip of the icerberg!

Timers

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

...

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

You're off to a good start because 1/ you want to improve 2/ you're noticing yourself the parts that can be improved in your code. Congratulations! So what can be improved here?

• You're manipulating your time as strings, which is unnecessary and dangerous. Strings are designed to be read by humans, not computers.
• You're not counting time, but the number of frames. On a computer which is twice as fast, time will fly twice as fast!
• You're not too careful about not mixing time logic and display logic.

Don't use time.asctime(), but time.gmtime():

import time
start_seconds = time.gmtime()

# In the main loop, look at elapsed time
seconds_elapsed = time.gmtime() - starts_seconds

# Somewhere else, print seconds_elapsed
screen.blit(...)

I didn't review other parts of your code.