I am a beginner in Python and PyGame and wrote a memory game. I think there are lot of things I could have done better. I am looking for some advice on how I can improve the design and refactoring of the code.

# the imports
import pygame
import random
from itertools import product
from pygame.locals import *
from pygame.color import Color

# the constants
FPS = 30

# the board size must be even due to pairs
assert (BOARD_HEIGHT * BOARD_WIDTH) % 2 == 0, 'The board size must be even'

# the shapes
DIAMOND = 'diamond'
SQUARE = 'square'
TRIANGLE = 'triangle'
CIRCLE = 'circle'

BGCOLOR = Color('blue')

# the main function
def main():
    global screen, clock


    screen = pygame.display.set_mode((SCREEN_WIDTH, SCREEN_HEIGHT))
    pygame.display.set_caption('Memory game')

    clock = pygame.time.Clock()

    colors = (Color('cyan'), Color('magenta'), Color('gray'), Color('chocolate'))

    # There should be enough symbols
    assert len(shape) * len(colors) >= BOARD_HEIGHT * BOARD_WIDTH // 2, 'There are not sufficient icons'

    board = get_random_board(shape, colors)
    revealed = [[False] * BOARD_WIDTH for i in range(BOARD_HEIGHT)]  # keeps track of visibility of square

    mouse_x = None
    mouse_y = None
    mouse_clicked = False
    first_selection = None

    running = True

    while running:
        draw_board(board, revealed)

        for event in pygame.event.get():
            if event.type == QUIT:
                running = False
            elif event.type == MOUSEMOTION:
                mouse_x, mouse_y = pygame.mouse.get_pos()
            elif event.type == MOUSEBUTTONDOWN:
                mouse_x, mouse_y = pygame.mouse.get_pos()
                mouse_clicked = True

        x, y = get_pos(mouse_x, mouse_y)

        if x is not None and y is not None:
            if not revealed[x][y]:
                if mouse_clicked:
                    revealed[x][y] = True
                    draw_square(board, revealed, x, y)

                    if first_selection is None:
                        first_selection = (x, y)
                        if board[x][y] != board[first_selection[0]][first_selection[1]]:
                            revealed[x][y] = False
                            revealed[first_selection[0]][first_selection[1]] = False
                        first_selection = None

                    if game_won(revealed):

                        game_won_animation(board, revealed)

                        board = get_random_board(shape, colors)
                        revealed = [[False] * BOARD_WIDTH for i in range(BOARD_HEIGHT)]

                    draw_select_box(x, y)

        mouse_clicked = False

def game_won(revealed):
    """ Returns whether all squares are found"""

    return all(all(x) for x in revealed)

def game_won_animation(board, revealed):
    """ Flashes background colors when the game is won"""

    color1 = Color('cyan')
    color2 = BGCOLOR
    for i in range(10):
        color1, color2 = color2, color1
        draw_board(board, revealed)

def start_game_animation(board):
    """Starts game by randomly showing 5 squares"""

    coordinates = list(product(range(BOARD_HEIGHT), range(BOARD_WIDTH)))

    revealed = [[False] * BOARD_WIDTH for i in range(BOARD_HEIGHT)]

    draw_board(board, revealed)

    for sz in range(0, BOARD_HEIGHT * BOARD_WIDTH, 5):
        l = coordinates[sz: sz + 5]
        for x in l:
            revealed[x[0]][x[1]] = True
            draw_square(board, revealed, *x)
        for x in l:
            revealed[x[0]][x[1]] = False
            draw_square(board, revealed, *x)

def get_random_board(shape, colors):
    """ Generates the board by random shuffling"""

    icons = list(product(shape, colors))
    num_icons = BOARD_HEIGHT * BOARD_WIDTH // 2
    icons = icons[:num_icons] * 2

    board = [icons[i:i + BOARD_WIDTH]
             for i in range(0, BOARD_HEIGHT * BOARD_WIDTH, BOARD_WIDTH)]
    return board

def get_coord(x, y):
    """ Gets the coordinates of particular square.
        The squares are number height wise and then width wise.
        So the x and y are interchanged."""

    left = Y_MARGIN + x * (SQUARE_SIZE + SQUARE_GAP)
    return top, left

def draw_icon(icon, x, y):
    """Draws the icon of (x, y) square"""

    px, py = get_coord(x, y)
    if icon[0] == DIAMOND:
        pygame.draw.polygon(screen, icon[1],
                            ((px + SQUARE_SIZE // 2, py + 5), (px + SQUARE_SIZE - 5, py + SQUARE_SIZE // 2),
                             (px + SQUARE_SIZE // 2, py + SQUARE_SIZE - 5), (px + 5, py + SQUARE_SIZE // 2)))
    elif icon[0] == SQUARE:
        pygame.draw.rect(screen, icon[1],
                         (px + 5, py + 5, SQUARE_SIZE - 10, SQUARE_SIZE - 10))
    elif icon[0] == TRIANGLE:
        pygame.draw.polygon(screen, icon[1],
                            ((px + SQUARE_SIZE // 2, py + 5), (px + 5, py + SQUARE_SIZE - 5),
                             (px + SQUARE_SIZE - 5, py + SQUARE_SIZE - 5)))
    elif icon[0] == CIRCLE:
        pygame.draw.circle(screen, icon[1],
                           (px + SQUARE_SIZE // 2, py + SQUARE_SIZE // 2), SQUARE_SIZE // 2 - 5)

def get_pos(cx, cy):
    """Gets the square (x, y) position  from the cartesian coordinates.
       The squares are number height wise and then width wise.
       So the cx and cy are interchanged."""

    if cx < X_MARGIN or cy < Y_MARGIN:
        return None, None

    x = (cy - Y_MARGIN) // (SQUARE_SIZE + SQUARE_GAP)
    y = (cx - X_MARGIN) // (SQUARE_SIZE + SQUARE_GAP)

        return None, None
        return x, y

def draw_square(board, revealed, x, y):
    """Draws a particular square"""

    coords = get_coord(x, y)
    square_rect = (*coords, SQUARE_SIZE, SQUARE_SIZE)
    pygame.draw.rect(screen, BGCOLOR, square_rect)
    if revealed[x][y]:
        draw_icon(board[x][y], x, y)
        pygame.draw.rect(screen, Color('gold'), square_rect)

def draw_board(board, revealed):
    """Draws the entire board"""

    for x in range(BOARD_HEIGHT):
        for y in range(BOARD_WIDTH):
            draw_square(board, revealed, x, y)

def draw_select_box(x, y):
    """Draws the highlight box around the square"""

    px, py = get_coord(x, y)
    pygame.draw.rect(screen, Color('red'), (px - 5, py - 5, SQUARE_SIZE + 10, SQUARE_SIZE + 10), 5)

if __name__ == '__main__':

From PEP 8, the Python style guide:

Wildcard imports (from <module> import *) should be avoided, as they make it unclear which names are present in the namespace, confusing both readers and many automated tools. There is one defensible use case for a wildcard import, which is to republish an internal interface as part of a public API (for example, overwriting a pure Python implementation of an interface with the definitions from an optional accelerator module and exactly which definitions will be overwritten isn't known in advance).

You aren't using magic values. Yipperdeoble! You define constants at the beginning of the file, instead. That's just great!

I spy with my little eye some global variables. Global variables are almost never needed. Whenever you want to use a global variable, you should ask yourself how many functions use this variable. If it is just one function (or at module-level) and a function it calls, you should be using arguments to the function and return values. If it matters to many functions, those functions are probably closely-enough related that they should be put within a class. The global variable should then be converted to an instance attribute. In this case, I would use a class.

I see that you use list comprehensions when you create a list with many of the same sublists. That's terrific! If you were to use list multiplication instead, you would just have so many references to the same list. I would suggest one small thing, though. It is common practice to use an underscore (_) as the name for a variable that is not used (such as in [False] * BOARD_WIDTH for i in ...). That makes it more clear to future readers that it is unused.

You use is and is not when comparing to None. Many people don't do that, but it is indeed recommended by PEP 8. Keep up the good work!

I see that you are familiar with all() and generator expressions. Your game_won() function is to be admired. That is the most efficient way.

Your start_game_animation() function converts product(...) to a list. That is unnecessary. Instead, use another itertools function for the slicing:

from itertools import product, islice


def start_game_animation(board):
    coordinates = product(...)
    for l in islice(coordinates, 0, BOARD_HEIGHT * BOARD_WIDTH, 5):

That is more efficient because you aren't storing all of that information in memory. While we are dealing with that code, l isn't a very descriptive name. It is also so similar to 1 that you are apt to confuse future readers. (It is even mentioned specifically in PEP 8 not to use l, O or I as variable names because of the digits that they look like in some fonts).

Your get_random_board() function could be improved (in efficiency) by using multiple functions from itertools:

icons = product(shape, colors)
num_icons = BOARD_HEIGHT * BOARD_WIDTH // 2
icons = list(islice(icons, None, num_icons)) * 2

return [icons[i: i + BOARD_WIDTH]
        for i in range(0, BOARD_HEIGHT * BOARD_WIDTH, BOARD_WIDTH)]

There is more that could be done that doesn't involve lists at all, but considering the size of the board, it would only barely affect the performance and is a little hard to understand.

Your draw_icon() function has many very long lines. I would suggest splitting them up a little bit. You could even define variables that would make it more clear what different parts of the equation are. You also figure out icon[0] once for each shape. Instead, define a variable and use it. You also don't account for the times when the shape is not one you have defined. I would suggest that you raise an error. Your function doesn't return anything, so it needs something else to tell the caller that he gave the wrong arguments to your function.

I am happy to see that you have an if __name__ == '__main__': block. That is something that is left out by many.

  • \$\begingroup\$ Thanks for spending your valuable time to review my code. I took note of all your points and changed it. But when I changed the coordinate creation line in the start_game_animation function to not include list function, I get an error that I couldn't use the random.shuffle() function. So How could I shuffle a generator? \$\endgroup\$
    – Boopesh
    May 9 '16 at 5:32
  • \$\begingroup\$ @Boopesh: I forgot about that part. It is probably better just to stick with the list. Since you know how many items the generator will produce, however, you could use coordinates = sample(coordinates, BOARD_HEIGHT * BOARD_WIDTH) \$\endgroup\$
    – zondo
    May 9 '16 at 10:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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