I made a menu to display a list of games and their info so that they can be uniformly launched from an arcade cabinet like set up. I was asked to do it in Python so I learned python using this and this. I have a Java background but no prior Python experience.

I have a working product but I realize that my code doesn't quite look like other Python I've seen. Mainly there is no main and there is no use of self. I just used the structure of the pygame tutorial. I'm looking for feedback on code correctness, best practices and design pattern usage.

If you propose major changes, what are the tools to help make them in Python? I'm used to Eclipse's refactoring tools for Java. Right now, I'm switching between Notepad++ and Sublime for Python. Other than find and replace, I don't know how to error free rename variables. How do I extract a method?

This is how it looks like:

enter image description here

import pygame
import json
import collections
import os

screen = pygame.display.set_mode((800, 600))
clock = pygame.time.Clock()
done = False

_image_library = {}

def create_image(path):
    global _image_library
    image = _image_library.get(path)
    if image == None:
            image = pygame.image.load(path)
            _image_library[path] = image

def get_image(path):
    global _image_library
    image = _image_library.get(path)
    if image == None:
            image = pygame.image.load(path)
            _image_library[path] = image
    return image

def draw_image(image, x, y):
    screen.blit(image, (x, y))

#font = pygame.font.SysFont("comicsansms", 72)
#text = font.render("Hello, World", True, (0, 128, 0))
size = 20
font = pygame.font.Font(None, size)
_text_library = {}

def creat_text(text, color):
    global _text_library
    color = (0, 128, 0)
    _text_library[text] = font.render(text, True, color)

def get_text(text):
    global _text_library
    color = (0, 128, 0)
    s = _text_library.get(text)
    if s == None:
        s = font.render(text, True, color)
        _text_library[text] = s
    return s

def draw_text(text, x, y):
    screen.blit(text, (x, y))

def draw_text_centered(text, x, y):
        (x - text.get_width() // 2, y - text.get_height() // 2))

def draw_rect(x, y, width, height, color):
    pygame.draw.rect(screen, color, pygame.Rect(x, y, width, height))

_games = collections.OrderedDict()
_DATA_I = 0
_X_I = 1
_Y_I = 2

_Y_TOP = 60
_DELTA_Y = 50
def load():
    global _games
    global _DATA_I
    global _X_I 
    global _Y_I 

    global _Y_TOP
    global _DELTA_Y

    f = open('./games.json')
    s = f.read()
    j = json.loads(s)

    x = 100
    y = _Y_TOP
    y_space = _DELTA_Y
    for game in j['games']:
        _games[game['Name']] = [game, x, y]
        y += y_space

    #Jet Pac is used as a generic key. The key used does not matter as long as it has all the fields to be displayed
    lables = _games['Jet Pac'][_DATA_I].keys()
    for text in lables:
        creat_text(text + ': ', None)

    print 'debug:'
    print _games
    for game in _games:
        print '\t' + game
        for dict_key in _games[game][_DATA_I].keys():
            print '\t\t' + dict_key + ': ' + _games[game][_DATA_I][dict_key]
            creat_text(_games[game][_DATA_I][dict_key], None)

    print _games['Jet Pac'][_DATA_I]['Name']
    print _text_library.keys()
    #end load

rect_x = 10
rect_y = RECT_Y_TOP
rect_width = 200
rect_height = 40
color = (50, 50, 50)
selected = _games.items()[0]
print '\n\n', selected
selected_i = 0
selected_data = _games.items()[selected_i][4][0]
print '\n', selected_data
while not done:
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            done = True
        if event.type == pygame.KEYDOWN and event.key == pygame.K_ESCAPE:
            done = True

    pressed = pygame.key.get_pressed()
    if pressed[pygame.K_UP]: 
        if not rect_y <= RECT_Y_TOP: 
            rect_y -= _DELTA_Y
            selected_i -= 1
            selected_data = _games.items()[selected_i][5][0]
        elif selected_i != 0:
            selected_i -= 1
            selected_data = _games.items()[selected_i][6][0]
            for game in _games:
                print game
                _games[game][_Y_I] -= _DELTA_Y
    if pressed[pygame.K_DOWN]:
        if selected_i != len(_games.items())-1:
            rect_y += _DELTA_Y
            selected_i += 1
            selected_data = _games.items()[selected_i][7][0]

    if pressed[pygame.K_RETURN]:
        print selected_data['exe']

    #begin draw
    screen.fill((0, 0, 0))
    draw_rect(rect_x, rect_y, rect_width, rect_height, color)
    #list games
    for game_name in _games:
        #if _games[game_name][_Y_I] >= _Y_TOP and _games[game_name][_Y_I] < RECT_Y_BOT:
            draw_text_centered(_text_library[game_name], _games[game_name][_X_I], _games[game_name][_Y_I])
    #list selected details
    x = 400
    y = 50
    ydif = 40

    draw_text(get_text('Name: ' + selected_data['Name']), x, y)
    y += ydif
    draw_text(get_text('Developer: ' + selected_data['Developer']), x, y)
    y += ydif
    draw_text(get_text('Publisher: ' + selected_data['Publisher']), x, y)
    y += ydif
    draw_text(get_text('Year: ' + selected_data['Year']), x, y)
    y += ydif
    draw_text(get_text('System: ' + selected_data['System']), x, y)
    y += ydif
    draw_text(get_text('Genre: ' + selected_data['Genre']), x, y)
    y += ydif
    draw_text(get_text('Players: ' + selected_data['Players']), x, y)
    y += ydif
    draw_text(get_text('Screenshot: '), x, y)
    y += ydif
    draw_image(get_image(selected_data['Screenshot']), x, y)


The game list is loaded from a JSON file here.


Global variables

Global variables are bad. Avoid them as much as possible.

One way to avoid them is by making them method parameters. But I see that it would make create_image and get_image inconvenient if you had to add _image_library in every call. A good solution can be to create an ImageLibrary class, which can contain _image_library, hitting two birds with one stone: 1. no more global variable; 2. still convenient

Similarly, you could create a TextLibrary.

For more on classes, see the Python tutorial (version 3), or version 2 if that's a requirement of pygame.

Code organization

As you noticed yourself, you don't have a main method. It's not a requirement, but a common place to put the glue-code that configures the main objects and launches the main activity of the program.

It's not good to have any code in the global namespace, except some global constants. This way the module cannot be imported and thus reused by other modules. So it's better to move the main glue code inside a main function, and add this boilerplate to call it:

if __name__ == '__main__':

This condition will only be true when running the script in a shell, it won't be true when importing as a module.

File I/O

Instead of this:

f = open('./games.json')
s = f.read()

The recommended pattern is this:

with open('./games.json') as fh:
    content = fh.read()

Notice that you don't need to close the file handle, it gets closed automatically when execution leaves the with block.

Also, s was a poor name, it's better to use meaningful names.

IDE and tools for Python

I recommend PyCharm, by the same company that makes IntelliJ. It's awesome, and would help your learning if you follow its warnings about your code. It would certainly help with refactoring operations.

PEP8 is the official coding style guide for Python, and there is a command line tools named pep8 (install with pip install pep8) which can check your entire project for violations.

Another tool is pyflakes (install with pip install pyflakes), which can detect some bad practices and potential bugs.

| improve this answer | |
  • 1
    \$\begingroup\$ Thank you. I posted a follow up (codereview.stackexchange.com/questions/64831/…) I got a student license of pycharm. I'm not thrilled with it but its better than pydev (eventhough I love eclipse for java). Still, sublime with ctrl shift v is much better at pasting and pydev reformats like I'd expect from eclipse. \$\endgroup\$ – Old Badman Grey Oct 6 '14 at 1:28

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.