Sign in to follow this  
Zyndrof

Python/pygame: Please review my code

Recommended Posts

Zyndrof    163
Hi! I just finished my first complete pygame application and I'm pretty happy with the way it turned out. But, I feel it loads very slow and before I start my next project I would like you to help me review my coding style and give some tips and pointers on what to look out for, what to keep doing and so on. Well, you've got more experience than I do, so I just post my code of 273 lines and you can browse it if you like. Oh, and it's a Tic-Tac-Toe game ^^ Thank you! :)
# Import and initialize pygame
import pygame
pygame.init()

# Set up the window
SCREEN = pygame.display.set_mode( ( 302, 302 ) )
pygame.display.set_caption( "Tic-Tac-Toe" )
BACKGROUND = pygame.Surface( SCREEN.get_size() )

# Keep track of what fields are marked and which ones
# are not.
SEL_FIELD = ["", "", "", "", "", "", "", "", ""]

# Who is playing?
PL = "1"

# Is the game over?
GAMEOVER = False

def createFields():
    FIELDCOLOR = (255, 255, 255)
    FIELDSIZE = pygame.Surface( ( 100, 100 ) ).convert()
    
    # Create and draw the upper row
    FIELD1 = FIELDSIZE
    FIELD1.fill( FIELDCOLOR )
    SCREEN.blit( FIELD1, ( 0, 0 ) )
    
    FIELD2 = FIELDSIZE
    FIELD2.fill( FIELDCOLOR )
    SCREEN.blit( FIELD2, ( 101, 0 ) )
    
    FIELD3 = FIELDSIZE
    FIELD3.fill( FIELDCOLOR )
    SCREEN.blit( FIELD3, ( 202, 0 ) )
    
    # Create and draw the middle row
    FIELD4 = FIELDSIZE
    FIELD4.fill( FIELDCOLOR )
    SCREEN.blit( FIELD4, ( 0, 101 ) )
    
    FIELD5 = FIELDSIZE
    FIELD5.fill( FIELDCOLOR )
    SCREEN.blit( FIELD5, ( 101, 101 ) )
    
    FIELD6 = FIELDSIZE
    FIELD6.fill( FIELDCOLOR )
    SCREEN.blit( FIELD6, ( 202, 101 ) )
    
    # Create and draw the bottom row
    FIELD7 = FIELDSIZE
    FIELD7.fill( FIELDCOLOR )
    SCREEN.blit( FIELD7, ( 0, 202 ) )
    
    FIELD8 = FIELDSIZE
    FIELD8.fill( FIELDCOLOR )
    SCREEN.blit( FIELD8, ( 101, 202 ) )
    
    FIELD9 = FIELDSIZE
    FIELD9.fill( FIELDCOLOR )
    SCREEN.blit( FIELD9, ( 202, 202 ) )
    
def clickedWhatField( ( POSX, POSY ) ):
    # Recives the x and y position of the cursor and
    # returns the fieldname.
    global PL, GAMEOVER
    
    # Who is playing?
    CURRENT = PL
    
    if POSX >= 0 and POSX <= 100:
        
        if POSY >= 0 and POSY <= 100:
            FIELD = 1
            
        elif POSY >= 101 and POSY <= 201:
            FIELD = 4
            
        elif POSY >= 202 and POSY <= 302:
            FIELD = 7
    
    elif POSX >= 101 and POSX <= 201:
        
        if POSY >= 0 and POSY <= 100:
            FIELD = 2
            
        elif POSY >= 101 and POSY <= 201:
            FIELD = 5
            
        elif POSY >= 202 and POSY <= 302:
            FIELD = 8
            
    elif POSX >= 202 and POSX <= 302:
        
        if POSY >= 0 and POSY <= 100:
            FIELD = 3
            
        elif POSY >= 101 and POSY <= 201:
            FIELD = 6
            
        elif POSY >= 202 and POSY <= 302:
            FIELD = 9
    
    # Send the information
    if not GAMEOVER:
        markField( CURRENT, FIELD )
            
def markField( PLAYER, FIELDNUMBER ):
   
    # We need global access to the player tracker
    global PL, SEL_FIELD, SCREEN
    
    # Assign the field to the current player.
    FIELD_IS_SET = SEL_FIELD[ FIELDNUMBER - 1 ]
    
    if len( FIELD_IS_SET ) == 0:
        SEL_FIELD[ FIELDNUMBER - 1 ] = "PL" + PLAYER
    
        # Set up variables
        PLACEMENT = 0
        LOCATION = ""
        
        if PLAYER == "1":
            LOCATION = "data/player1.png"
            PL = "2"
        else:
            LOCATION = "data/player2.png"
            PL = "1"
            
        # Set the placement variable
        if FIELDNUMBER == 1:
            PLACEMENT = ( 0, 0 )
        elif FIELDNUMBER == 2:
            PLACEMENT = ( 101, 0 )
        elif FIELDNUMBER == 3:
            PLACEMENT = ( 202, 0 )
        elif FIELDNUMBER == 4:
            PLACEMENT = ( 0, 101 )
        elif FIELDNUMBER == 5:
            PLACEMENT = ( 101, 101 )
        elif FIELDNUMBER == 6:
            PLACEMENT = ( 202, 101 )
        elif FIELDNUMBER == 7:
            PLACEMENT = ( 0, 202 )
        elif FIELDNUMBER == 8:
            PLACEMENT = ( 101, 202 )
        elif FIELDNUMBER == 9:
            PLACEMENT = ( 202, 202 )
            
        try:
            IMAGE = pygame.image.load( LOCATION ).convert()
            SCREEN.blit( IMAGE, PLACEMENT )
            
            # Did anybody win?
            checkWin()
        except:
            print "Couldn't load image"
        
def checkWin():
    """ Checks if there is a winner and in that case, show it """
    global BACKGROUND, GAMEOVER
    
    WINNER = ""
    
    # Check if player 1 has three in a row.
    if SEL_FIELD[0] == "PL1" and SEL_FIELD[1] == "PL1" and SEL_FIELD[2] == "PL1":
        WINNER = "PL1"
    
    elif SEL_FIELD[3] == "PL1" and SEL_FIELD[4] == "PL1" and SEL_FIELD[5] == "PL1":
        WINNER = "PL1"
        
    elif SEL_FIELD[6] == "PL1" and SEL_FIELD[7] == "PL1" and SEL_FIELD[8] == "PL1":
        WINNER = "PL1"
        
    elif SEL_FIELD[0] == "PL1" and SEL_FIELD[3] == "PL1" and SEL_FIELD[6] == "PL1":
        WINNER = "PL1"
        
    elif SEL_FIELD[1] == "PL1" and SEL_FIELD[4] == "PL1" and SEL_FIELD[7] == "PL1":
        WINNER = "PL1"
        
    elif SEL_FIELD[2] == "PL1" and SEL_FIELD[5] == "PL1" and SEL_FIELD[8] == "PL1":
        WINNER = "PL1"
        
    elif SEL_FIELD[0] == "PL1" and SEL_FIELD[4] == "PL1" and SEL_FIELD[8] == "PL1":
        WINNER = "PL1"
        
    elif SEL_FIELD[2] == "PL1" and SEL_FIELD[4] == "PL1" and SEL_FIELD[6] == "PL1":
        WINNER = "PL1"
        
    # Check if player 2 has three in a row.
    if SEL_FIELD[0] == "PL2" and SEL_FIELD[1] == "PL2" and SEL_FIELD[2] == "PL2":
        WINNER = "PL2"
    
    elif SEL_FIELD[3] == "PL2" and SEL_FIELD[4] == "PL2" and SEL_FIELD[5] == "PL2":
        WINNER = "PL2"
        
    elif SEL_FIELD[6] == "PL2" and SEL_FIELD[7] == "PL2" and SEL_FIELD[8] == "PL2":
        WINNER = "PL2"
        
    elif SEL_FIELD[0] == "PL2" and SEL_FIELD[3] == "PL2" and SEL_FIELD[6] == "PL2":
        WINNER = "PL2"
        
    elif SEL_FIELD[1] == "PL2" and SEL_FIELD[4] == "PL2" and SEL_FIELD[7] == "PL2":
        WINNER = "PL2"
        
    elif SEL_FIELD[2] == "PL2" and SEL_FIELD[5] == "PL2" and SEL_FIELD[8] == "PL2":
        WINNER = "PL2"
        
    elif SEL_FIELD[0] == "PL2" and SEL_FIELD[4] == "PL2" and SEL_FIELD[8] == "PL2":
        WINNER = "PL2"
        
    elif SEL_FIELD[2] == "PL2" and SEL_FIELD[4] == "PL2" and SEL_FIELD[6] == "PL2":
        WINNER = "PL2"   
    
    # Did anybody win this time?
    if WINNER == "PL1":
        # Create a label
        FONT = pygame.font.Font( None, 36 )
        TEXT = FONT.render( "Player 1 wins!", 1, ( 10, 10, 10 ) )
        TEXTPOS = TEXT.get_rect()
        TEXTPOS.centerx = BACKGROUND.get_rect().centerx
        TEXTPOS.centery = BACKGROUND.get_rect().centery
        SCREEN.blit( TEXT, TEXTPOS )
        
        # End the game
        GAMEOVER = True
        
    elif WINNER == "PL2":
        # Create a label
        FONT = pygame.font.Font( None, 36 )
        TEXT = FONT.render( "Player 2 wins!", 1, ( 10, 10, 10 ) )
        TEXTPOS = TEXT.get_rect()
        TEXTPOS.centerx = BACKGROUND.get_rect().centerx
        TEXTPOS.centery = BACKGROUND.get_rect().centery
        SCREEN.blit( TEXT, TEXTPOS )
        
        # End the game
        GAMEOVER = True

def main():
    # Set up the background information
    global BACKGROUND
    BACKGROUND = BACKGROUND.convert()
    BACKGROUND.fill( ( 0, 0, 0 ) )
    
    SCREEN.blit( BACKGROUND, ( 0, 0 ) )
    
    # Create the fields
    createFields()
    
    # Create key variables to set up the main loop
    KEEPGOING = True
    CLOCK = pygame.time.Clock()
    
    # Set up the main loop
    while KEEPGOING:
        # Set frame rate
        CLOCK.tick( 30 )
        
        # Event handling
        for event in pygame.event.get():
            if event.type == pygame.QUIT:
                KEEPGOING = False
            
            elif event.type == pygame.MOUSEBUTTONDOWN:
                 clickedWhatField( pygame.mouse.get_pos() )
        
        # Refresh the screen
        pygame.display.flip()

# Only run the program from within main    
if __name__ == "__main__":
    main()
[Edited by - Zyndrof on August 2, 2007 7:14:17 AM]

Share this post


Link to post
Share on other sites
Barius    325
some comments, in no particular order:

- Don't use CAPITAL VARIABLE NAMES so much.
- You're loading the X/O image every turn. This is unnecessary, you could load them just once at the start of the game.
- You're "hardcoding" a lot of your game logic, using long chains of "if" statements. In TicTacToe, this is easy to do as there're relatively few fields and combinations to check, but it is a bad habit to get into, because the number of combinations you need to check grows explosively with more complex games. Try to use "for" loops more, and calculate values directly. For example, I'd rewrite createFields as

def createFields():
color = (255, 255, 255)
width = 100
height = 100
field = pygame.Surface((width, height)).convert()
field.fill(color)
for x in range(3): # go from 0 to 2
for y in range(3):
SCREEN.blit(field, ((width+1)*x, (height+1)*y))

Perhaps implementing Connect Four or Gomoku will convince you of the utility of arithmetic and "for" loops :-)

- Global variables are considered bad style because they can make larger programs hard to understand. It would be a good idea to pass data as parameters just to the functions that need it.





Share this post


Link to post
Share on other sites
Zyndrof    163
Thank you!

I will take that into account and rewrite the game so that it can have more than three rows. You could for example have four or five rows.

Share this post


Link to post
Share on other sites
realcore    134
Another note, not really about how you've done your code, but more about the game itself. First off, it does not restart if one player wins, nor does it quit. An option to do either of these would be nice.
Second, it's quite possible to fill the entire board without there being a winner. You should probably check for that as well.

Share this post


Link to post
Share on other sites
Zyndrof    163
Restart: I did a function for this that caused me some problems. When a user won, the game restarted after the 2 seconds I wanted it to, BUT the last selected field wasn't filled. So I diched that code, at least for now.

About filling the entire board: I didn't even think of ending the game in the occurance of a tie. Will look into that.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this