3
\$\begingroup\$

I managed to finish this game I am sure you have encountered before. If you run it there are instructions on what to do. Any code suggestions / improvements?

import pygame,random,math
from time import sleep

pygame.init()

#window resolution
screen_width = 800
screen_height = 800

screen = pygame.display.set_mode( ( screen_width,screen_height ) )
pygame.display.set_caption( 'The Flash Game - Marek Borik' )

n_squares_in_row = 7
line_thickness = 20 #pixels

wait_duration = 1.5 #seconds
flash_duration = 0.3 #seconds

fontsize = screen_width // 25
rounds = 10 #rounds of flashes

#defining colors
RED = ( 255,0,0 )
BLUE = ( 0,0,255 )
WHITE = ( 255,255,255 )
BLACK = ( 0,0,0 )


def TextObjects( string,font,color ):
    textSurface = font.render( string,True,color )
    return textSurface,textSurface.get_rect()


def DrawText( string,fontSizeX,color,x,y ):
    text = pygame.font.SysFont( "timesnewroman",int( math.ceil( fontSizeX ) ) )
    TextSurf, TextRect = TextObjects( string,text,color )
    TextRect.center = ( x,y )
    screen.blit( TextSurf,TextRect )


def GenerateColorInfo( n_squares_in_row ):
    n_squares_total = n_squares_in_row ** 2

    not5050 = True #Prevents from having the same amount of red and blue squares if applicable ( e.g. field with dimentions 7 x 7 squares squares will never have this issue )

    while( not5050 ):
        color_info = []

        for i in range( n_squares_total + 1 ):
            rand = random.randint( 0,1 )
            color_info.append( rand )

        counter_red = 0
        counter_blue = 0

        for i in color_info:
            if i == 0:
                counter_red += 1
            if i == 1:
                counter_blue += 1

        if counter_red == n_squares_total // 2 or counter_blue == n_squares_total // 2 or counter_red == counter_blue:
            not5050 = True
        else:
            not5050 = False


    return color_info,counter_red,counter_blue


def DrawSquares( n_squares_in_row,color_info,square_width,square_height ):
    for i in range( n_squares_in_row ):
        for j in range( n_squares_in_row ):
            if color_info[ i * n_squares_in_row + j ] == 0:
                pygame.draw.rect( screen,RED,( j * square_width,i * square_height,j * square_width + square_width,i * square_height + screen_height ),0 )
            else:
                pygame.draw.rect( screen,BLUE,( j * square_width,i * square_height,j * square_width + square_width,i * square_height + screen_height ),0 )


def DrawGrid( n_squares_in_row,line_thickness,square_width,square_height ):
    for i in range( 1,n_squares_in_row ):
        pygame.draw.line( screen,BLACK,( i * square_width,0 ),( i * square_width,screen_height ),line_thickness )

    for i in range( 1,n_squares_in_row ):
        pygame.draw.line( screen,BLACK,( 0,i * square_height ),( screen_height,i * square_width ),line_thickness )

    pygame.draw.line( screen,BLACK,( 0,0 ),( 0,screen_height ),line_thickness )
    pygame.draw.line( screen,BLACK,( 0,0 ),( screen_width,0 ),line_thickness )
    pygame.draw.line( screen,BLACK,( screen_width,0 ),( screen_width,screen_height ),line_thickness )
    pygame.draw.line( screen,BLACK,( 0,screen_height ),( screen_width,screen_height ),line_thickness )


def DrawStartScreen():
    end = False

    while not end:
        for event in pygame.event.get():
            if event.type == pygame.KEYDOWN:
                if event.key == pygame.K_SPACE:
                    end = True
            if event.type == pygame.QUIT:
                pygame.quit()
                quit()

        screen.fill( BLACK )

        DrawText( "The Flash Game - Created by Marek Borik",fontsize * 1.4,WHITE,screen_width * 0.5,screen_height * 0.05 )

        DrawText( "This game will test your subconscious perception.",fontsize,WHITE,screen_width * 0.5,screen_height * 0.2 )
        DrawText( ( "You will be shown a grid of red and blue squares " + str( rounds ) + " times." ),fontsize,WHITE,screen_width * 0.5, screen_height * 0.25 )
        DrawText( "Your task is to determine if you saw more red or blue circles.",fontsize,WHITE,screen_width * 0.5, screen_height * 0.3 )

        DrawText( "If you see more:",fontsize,WHITE,screen_width * 0.3,screen_height * 0.45 )
        DrawText( "If you see more:",fontsize,WHITE,screen_width * 0.7,screen_height * 0.45 )

        pygame.draw.rect( screen,RED,( screen_width * 0.2, screen_height * 0.5,screen_width * 0.2,screen_height * 0.2 ),0 )
        pygame.draw.rect( screen,BLUE,( screen_width * 0.6, screen_height * 0.5,screen_width * 0.2,screen_height * 0.2 ),0 )

        DrawText( "Press Left Arrow",fontsize,WHITE,screen_width * 0.3,screen_height * 0.75 )
        DrawText( "Press Right Arrow",fontsize,WHITE,screen_width * 0.7,screen_height * 0.75 )

        DrawText( "Press spacebar to start",fontsize,WHITE,screen_width * 0.5,screen_height * 0.93 )


        pygame.display.update()


def DoRound():
    left_arrow = False
    right_arrow = False
    turn = False

    #if the windows isn't square, then squares are not squares and we need to treat them like rectangles
    square_width = screen_width / n_squares_in_row
    square_height = screen_height / n_squares_in_row

    color_info,n_red,n_blue = GenerateColorInfo( n_squares_in_row )

    screen.fill( BLACK )
    pygame.display.update()

    sleep( wait_duration )

    #flicker squares for the flash duration

    DrawSquares( n_squares_in_row,color_info,square_width,square_height )
    DrawGrid( n_squares_in_row,line_thickness,square_width,square_height )

    pygame.display.update()

    sleep( flash_duration )
    screen.fill( BLACK )
    pygame.display.update()

    while not turn: # after flash wait for the turn to be completed by pressing either arrow to indicate the answer
       for event in pygame.event.get():
            if event.type == pygame.KEYDOWN:
                if event.key == pygame.K_LEFT:
                    left_arrow = True
                    turn = True
                if event.key == pygame.K_RIGHT:
                    right_arrow = True
                    turn = True

            if event.type == pygame.QUIT:
                pygame.quit()
                quit()

    if n_red >= n_blue and left_arrow == True:  #if the arrow coresponds to the correct answer, return 1, else 0
        return 1

    elif n_blue >= n_red and right_arrow == True:
        return 1

    else:
        return 0


def Game():
   guesses = []

    for i in range( rounds ):
        guesses.append( DoRound() )

    return guesses.count( 1 ) #count ones, meaning correct answers


def EndScreen( correct ):
    screen.fill( BLACK )

    DrawText( "You got " + str( correct ) + " / " + str(  rounds ) + " correct!",fontsize * 2,WHITE,screen_width * 0.5,screen_height * 0.5 )

    pygame.display.update()

    end = False

    while not end:
        for event in pygame.event.get():
            if event.type == pygame.KEYDOWN:
                if event.key == pygame.K_SPACE:
                    end = True
            if event.type == pygame.QUIT:
                pygame.quit()
                quit()


DrawStartScreen()
correct = Game()
EndScreen( correct )
pygame.quit()
quit()
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Good job on the separation of concerns

I was very happy to see a function that deals only with the logic and some functions that deal with the drawing and interaction. It made it easy for me to test analyse and simplify it.

generate_color_info

The function was too complex and verbose for the simplicity of its task.

  • The not5050 variable is not necessary (you can use the condition directly).
  • Verbose explicit loops were used in place of list comprehensions
  • The built-in list.count was ignored.

I proceeded in the re-factoring by writing a test first (runnable with doctest) and then reimplementing it more simply::

def GenerateColorInfo( n_squares_in_row ):
    """
    >>> random.seed(0)
    >>> GenerateColorInfo( 5 )
    ([1, 1, 0, 0, 1, 0, 1, 0, 0, 1, 1, 1, 0, 1, 1, 0, 1, 1, 1, 1, 0, 1, 1, 1, 0, 0], 10, 16)
    """
    while True:
        randoms = [random.randint(0, 1) for _ in range(n_squares_in_row ** 2 + 1)]
        if randoms.count(0) != randoms.count(1):
            break
    return randoms, randoms.count(0), randoms.count(1)

List comprehension in Game

Game was unnecessarily long:

def Game():
    return [DoRound() for _ in range( rounds )].count(True)

Boolean simplification

if n_red >= n_blue and left_arrow == True:  #if the arrow coresponds to the correct answer, return 1, else 0
    return 1

elif n_blue >= n_red and right_arrow == True:
    return 1

else:
    return 0

Is the same as

return (n_red >= n_blue and left_arrow) or \
       (n_blue >= n_red and right_arrow)

But the second is obviously simpler (also it is better because it returns True / False instead of 1 and 0 making it more clear it is a boolean function.)

Arbitrary memory task for the user

How were the right and left arrow assigned to red and blue? It is more reasonable to have the user enter R for Red and B for Blue. This way the connection is obvious and the user does not need to use memory.

Does the code handle rectangles?

Some code of yours seems to be made to handle rectangles:

#if the windows isn't square, then squares are not squares and we need to treat them like rectangles
square_width = screen_width / n_squares_in_row
square_height = screen_height / n_squares_in_row

Other code implies a square grid:

def DrawSquares( n_squares_in_row,color_info,square_width,square_height ):
    for i in range( n_squares_in_row ):
        for j in range( n_squares_in_row ):
            if color_info[ i * n_squares_in_row + j ] == 0:
                pygame.draw.rect( screen,RED,( j * square_width,i * square_height,j * square_width + square_width,i * square_height + screen_height ),0 )
            else:
                pygame.draw.rect( screen,BLUE,( j * square_width,i * square_height,j * square_width + square_width,i * square_height + screen_height ),0 )

Please either write all code to accommodate for rectangles or none at all, just some of it creates complexity and confusion for no gain.

Repetition

Your code states the same concepts many times:

            if color_info[ i * n_squares_in_row + j ] == 0:
                pygame.draw.rect( screen,RED,( j * square_width,i * square_height,j * square_width + square_width,i * square_height + screen_height ),0 )
            else:
                pygame.draw.rect( screen,BLUE,( j * square_width,i * square_height,j * square_width + square_width,i * square_height + screen_height ),0 )

The if and else are equal except for BLUE or RED as the color. The DRY principle is one of the most important of software development.

Here is how you can fix the duplication:

        color = RED if color_info[ i * n_squares_in_row + j ] == 0 else BLUE
        pygame.draw.rect( screen, color, ( j * square_width,i * square_height,j * square_width + square_width,i * square_height + screen_height ),0 )

Now the drawing code is called exactly once and it is obvious that only the colour changes.


pygame.draw.line( screen,BLACK,( 0,0 ),( 0,screen_height ),line_thickness )
pygame.draw.line( screen,BLACK,( 0,0 ),( screen_width,0 ),line_thickness )
pygame.draw.line( screen,BLACK,( screen_width,0 ),( screen_width,screen_height ),line_thickness )
pygame.draw.line( screen,BLACK,( 0,screen_height ),( screen_width,screen_height ),line_thickness )

In this block of code the segment pygame.draw.line( screen,BLACK, ... line_thickness) is repeated 4 times. Only the start and end points change, so we can use a loop:

for (start, end) in [ ( ( 0,0 ),( 0,screen_height) ),
                      ( ( 0,0 ),( screen_width,0 ) ),
                      (( screen_width,0 ),( screen_width,screen_height )),
                      (( 0,screen_height ),( screen_width,screen_height )) ]:
    pygame.draw.line(screen, BLACK, start, end, line_thickness)

When you DrawText fontsize is fontsize and color is WHITE, you could define this as keyword default arguments to reduce repetition at the cost of being explicit when other colors are needed.


These blocks of code are similar:

while not turn: # after flash wait for the turn to be completed by pressing either arrow to indicate the answer
   for event in pygame.event.get():
        if event.type == pygame.KEYDOWN:
            if event.key == pygame.K_LEFT:
                left_arrow = True
                turn = True
            if event.key == pygame.K_RIGHT:
                right_arrow = True
                turn = True

        if event.type == pygame.QUIT:
            pygame.quit()
            quit()

and

while not end:
    for event in pygame.event.get():
        if event.type == pygame.KEYDOWN:
            if event.key == pygame.K_SPACE:
                end = True
        if event.type == pygame.QUIT:
            pygame.quit()
            quit()

It should be possible to write a function once and use it twice.

Naming / Spacing

You may call this minor, but a very widely adopted style guide exists for Python (PEP8) stating that constants should be ALL_UPPERCASE and all other names lowercase_with_underscores. It helps increasing consistency between programs. I suggest running autopep8 on your code to fix the spacing to be consistent with widely accepted style.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you very much, I would just like to say that the variable not5050 does indeed work, for it makes the color generating cycle run again if there is the same number of red and blue squares. That is why I can't simplify it as you suggested, because I need to do all the counting and checking. \$\endgroup\$ Oct 4, 2016 at 17:39
  • \$\begingroup\$ @TheTask1337 I updated a new version that is a bit more complex but mimics the behaviour of your original function. \$\endgroup\$
    – Caridorc
    Oct 4, 2016 at 19:20
  • \$\begingroup\$ Great answer! Would like to add that your functions ( @Caridorc ) should be named with lowercase_with_underscores instead of CamelCase in order to follow PEP8 and not to confuse other coders, since CamelCase is often used for classes and not functions. \$\endgroup\$ Oct 12, 2016 at 12:38
  • \$\begingroup\$ @TedKleinBergman Yes that goes without saying \$\endgroup\$
    – Caridorc
    Oct 12, 2016 at 13:03
  • \$\begingroup\$ Sorry, taged the wrong person... Meant to tag @TheTask1337 \$\endgroup\$ Oct 12, 2016 at 13:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.