First Chess Project (ver 1.1)

Question

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:

1. 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.
                        if j.colour==colour:
                            selectedPiece=j
                            m.extend([x,y]) 

Could you please just go through the code and tell me if this is how Python is written in the industry?

Answer 1

Since you ask about production (industry) use, there is a long list.

Regarding the code design:

If we look carefully, there are 3 attributes that define a piece:

• Color: Black, White
• Piece Type: Bishop, King, Knight, Pawn, Queen, Rook
• Position on Board: (A-H, 1-8)

Here on I shall refer to Color` as color, Piece Type as type and Position on board as pos.

Your code is not DRY (Don't Repeat Yourself)

You are repeating yourself in code, like BlackPawn.check is the same as WhitePawn.check. This means if there is some problem in one such place you need to change the code everywhere to fix it. Trust me, it's not a good idea!

As the black and white pieces behave the same except for the killing which depends on the color, the class hierarchy can be changed to something like:

Piece
    Bishop
    King
    Knight
    Pawn
    Queen
    Rook

That sorts the type system.

Now pass the other 2 attributes that define a piece: color and pos in it's initializer. The image's name can be calculated from the type and color. Do this calculation in the Piece class, upon initialization. Make each subclass define the common part of the image name as a class attribute and add a '_b' to it when the piece is black.

You might want to add a class that manages or represent the chess-board That class check if the move is out of bounds, hitting another piece etc. This way your Pieces only have to tell the board what moves are valid for it from this position. Let the board decide if the pieces can make that move.

Also for pieces that travel a path, give it a nested list, of the moves that may become invalid if this one is. Like a bishop on B2 can't go to E5 if there is a pawn on D4. All moves till D4 are valid (different color) but not those beyond D4.

Relating code-style (in a production environment, Readability Matters!):

Note that I spent 200 minutes listing everything line by line like:

• Never use triple quoted strings for single line comments as you have on lines 422, 433. They are fine for comments without indent as on line 181.

Here's a much shorter list:

• It would be hard to test this code. For tips on testable code see this answer.

• Use the space-bar key in places other than the indentation, like around operators.

• Break up long lines across multiple lines.

• Follow PEP-8.

• Read these docs.

• One statement per line. Don't follow up an if statement with a statement simply because it's the only one.

Answer 2

This code seems much better than before, so congrats. Here are a few more things to adjust:

1. Move your main game loop into a function and call it in an if block like this:

   if __name__ = "__main__":
       #call your main function
   

   This is so that people (including yourself) importing your module don't immediately get a chess window popping up.

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

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

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