6
\$\begingroup\$

I have a medium-sized project here, and I would like some comments on the code. In particular I'd like comments on how well I'm handling OO-programming, my programs logic and whether you think my code is "Pythonic."

The project is up on GitHub. Currently, the only major features that are missing are a proper options menu (although basic menu's like the pause and start menu work).

Here's a basic rundown of how the program works:

We have a Game class which handles the context of pause menus and the game itself. It does this with its call() method, which runs another Game instance which takes over the screen. When the callee' exits, control returns to the caller and everything is set to how it was before the call was made (except for music playing, that doesn't work yet, but I have a solution for it.) In the case where quitGame() is given a string argument, control will be returned to the game named. For example, in the pause menu there is an option to "exit to main menu" which is handled like such: self.quitGame("MainMenu").

The drawing of objects and handling of events are related to what objects are currently registered with the running game, self.addJob("name", Class()) adds a job/object. There is currently no Job() class being derived from, this is something I've been meaning to add. Events are handled by each separate object, they all have an eventHandler() method which is called with the events returned from pygame.event.get() every loop. This means that keys that for example move the Tetromino on the screen will only work in the context of a Tetromino job being registered for the current game. Personally, I think this makes event handling very clean and easy to extend. For example, when the game is over, a TimedExecution job is added, with the anykey option set to True.

self.addJob("endtimer", TimedExecution(self.quitGame, seconds=2, anykey=True))

This means that when a key is pressed, it will immediately execute the function it was given, self.quitGame.

Another important part of the program is the TextBox class, which like the name advertises is a Job that will display a text box. It renders/caches fonts so that I never have to deal with that manually (except for when I wrote the TextBox itself.) The way it's called should explain it well.

TextBox(self, "Level: {level}\nScore: {score}\nLines: {lines}\nLines left: {level up}",
        border=True, y=BLOCK_HEIGHT+1, x=BLOCK_WIDTH*2+(BOARD_WIDTH)*BOARD_BLOCKWIDTH, textfit=True,
        colors={"border":(0xaa,0xaa,0xaa), "font":(0xaa,0xaa,0xaa)},
        font=TETRIS_STATUSBOX_FONT,
        variables={"level": lambda s: s.getJob("board").level,
                   "score": lambda s: s.getJob("board").score,
                   "lines": lambda s: s.getJob("board").lines,
                   "level up": lambda s: s.getJob("board").level_lines,
                  }
       )

TETRIS_STATUSBOX_FONT is a dictionary, that's how I chose to represent the fonts.

Most of the code is in Tetris.py, then there is some code for loading XML. Here you can download the entire thing as a zipped folder.

I was told to post my code directly, unfortunately there is a limit to the amount of characters I can have in my post. So I'll upload it to pastebin, where no changes will be made at least.

Pastebin

Tetromino class:

class Tetromino(object):
    def __init__(self, board, matrix, type, color, x=0, y=None, updateinterval=FRAMERATE, queue=0):
        self.matrix = matrix
        self.type = type
        self.board = board
        self.color = color
        self.updateinterval = updateinterval
        self.time_until_update = self.updateinterval
        self.draw_required = True
        self.update_required = True
        self.sped_up = False
        self.x = x
        self.y = y
        self.queue = queue
        self.level = 1

        if y == None:
            self.y = -(len(self.matrix))

    ## Hackety hack
    def forBlock(self, func, boolean=False):
        for y in xrange(len(self.matrix)):
            for x in xrange(len(self.matrix[y])):
                if self.matrix[y][x] and func(self.x + x, self.y + y, self.matrix) and boolean:
                    return True

    def draw(self):
        def drawBlock(x, y, _):
            self.board.drawCube(x, y, self.color)
        self.forBlock(drawBlock)

    def insert(self):
        def insert(x, y, _):
            self.board.blocks[(x, y)] = self.color

        if self.y < 0:
            ## XXX: GAME OVER
            self.board.update_required = False

        self.forBlock(insert)
        self.board.checkTetris()
        self.update_required = False

    def update(self):
        self.time_until_update -= 1
        if self.time_until_update <= 0:
            self.moveDiagonal(1)
            self.time_until_update = self.updateinterval

    def drop(self):
        while self.update_required:
            self.moveDiagonal(1)

    def checkBlockCollision(self):
        def colliding(x, y, _):
            return self.board.blocks.get((x, y))
        return self.forBlock(colliding, boolean=True)

    def checkWallCollision(self, xp, yp):
        for y in xrange(len(self.matrix)):
            for x in xrange(len(self.matrix[y])):
                ## Some of the functions need to know which edge the collision happened on,
                ## otherwise the result can be treated like a boolean.
                if self.matrix[y][x]:
                    if yp+y > self.board.height-1:
                        return "bottom"
                    if xp+x > self.board.width-1:
                        return "right"
                    if xp+x < 0:
                        return "left"

    ## Move diagonally, if possible
    def moveDiagonal(self, direction):
        self.y += direction
        if self.checkBlockCollision():
            self.y -= direction
            self.insert()
        if self.checkWallCollision(self.x, self.y) == "bottom":
            self.y -= direction
            self.insert()

    ## Move horizontally, if possible
    def moveHorizontal(self, direction):
        self.x += direction
        if self.checkBlockCollision():
            self.x -= direction
        if self.checkWallCollision(self.x, self.y):
            self.x -= direction

    ## Rotate if possible
    def rotate(self, direction):
        last_matrix = self.matrix
        self.matrix = rot90(self.matrix)
        if self.checkWallCollision(self.x, self.y) or self.checkBlockCollision():
            self.matrix = last_matrix

    ## It makes the game WAAY to easy, but i kind of always wondered "what if"
    def flip(self):
        flip(self.matrix)
        if self.checkWallCollision(self.x, self.y) or self.checkBlockCollision():
            flip(self.matrix)

    def eventHandler(self, events):
        for event in events:
            if event.type == KEYUP:
                if event.key == keymap["game"]["speed_up"] and self.sped_up:
                    self.sped_up = False
                    self.updateinterval *= 10
                    self.time_until_update = self.updateinterval

            if event.type == KEYDOWN:
                if event.key == keymap["game"]["rotate_right"]:
                    self.rotate(1)
                elif event.key == keymap["game"]["rotate_left"]:
                    self.rotate(-1)
                elif event.key == keymap["game"]["reverse"]:
                    self.flip()

                elif event.key == keymap["game"]["move_right"]:
                    self.moveHorizontal(1)
                elif event.key == keymap["game"]["move_left"]:
                    self.moveHorizontal(-1)

                elif event.key == keymap["game"]["drop_down"]:
                    self.drop()

                elif event.key == keymap["game"]["speed_up"]:
                    self.sped_up = True
                    self.updateinterval /= 10
                    self.time_until_update = self.updateinterval

Game class:

class Game(object):
    def __init__(self, _id, caption="", mouse_visible=True, bgcolor=(0x22,0x22,0x22), screen=None, ticktime=FRAMERATE,
                 width=SCREEN_WIDTH, height=SCREEN_HEIGHT, x=SCREEN_WIDTH, y=SCREEN_HEIGHT, sound_enabled=False, soundtrack=None):
        self.caption = caption
        self.mouse_visible = mouse_visible
        self.bgcolor = bgcolor
        self.screen = screen
        self.ticktime = ticktime
        self.batch = {}
        self.drawqueue = []
        self.ret = 0
        self.windows = {}
        self.height = y
        self.width = x
        self.events = None
        self.id = _id
        self.soundtrack = soundtrack
        self.sound_enabled = sound_enabled
        self.playing = ""

        self.setup()

    def stopMusic(self):
        self.playing = ""
        Pygame.mixer.music.stop()

    ## TODO: The call/quit model currently fails here, I'll just have to save the music's "progress."
    def playMusic(self, path, loops=1):
        try:
            if not self.sound_enabled:
                Log.warning("Attempted to play music in `{}' where sound has been disabled".format(self.id))
            Pygame.mixer.music.load(path)
            Pygame.mixer.music.play(loops)
            Log.log("Playing sountrack `{}'".format(path))
            self.playing = path
        except:
            Log.error("Unable to play music file: `{}'".format(path))

    def getJob(self, name):
        return self.batch[name]

    def addJob(self, name, obj):
        self.batch[name] = obj
        self.drawqueue.append(name)

    ## Why not just call Sys.exit(), why create a separate method for this?
    ## Because finishing of can get more complex as this program develops.
    def quit(self):
        Sys.exit()

    ## We just "exploit" the stack to create things like pause menus or other "contexts"
    ## that take over the screen.
    def call(self, obj, **kwargs):
        game = obj(screen=self.screen, **kwargs)
        ret = game.run()

        self.setup()

        if ret and self.id != ret:
            self.quitGame(ret)

    def quitGame(self, *args):
        if args:
            self.ret = args[0]
        if self.playing:
            self.stopMusic()
        self.running = None

    def setup(self):
        Pygame.init()
        Pygame.display.set_caption(self.caption)
        Pygame.mouse.set_visible(int(self.mouse_visible))
        if not Pygame.mixer.get_init() and self.sound_enabled:
            Log.log("Initializing mixer")
            Pygame.mixer.init()
        if self.soundtrack and self.sound_enabled and not self.playing:
            self.playMusic(self.soundtrack, loops=-1)
        if not self.screen:
            self.screen = Pygame.display.set_mode((self.width, self.height), DISPLAY_OPTIONS)
        self.screen.fill(self.bgcolor)
        Pygame.display.flip()
        self.clock = Pygame.time.Clock()

    def eventHandler(self, events):
        pass

    def run(self):
        if not hasattr(self, "running") or not hasattr(self, "eventHandler"):
            raise GameError("Game has not been properly initialized")

        while self.running:
            self.clock.tick(self.ticktime)
            self.screen.fill(self.bgcolor)
            self.events = Pygame.event.get()
            queue = sorted(self.batch, key=lambda obj: self.batch[obj].queue)
            for obj in queue:
                obj = self.getJob(obj)
                if obj.update_required:
                    obj.update()
                if obj.draw_required:
                    obj.draw()

                ## Context is love, context is life.
                obj.eventHandler(self.events)
            Pygame.display.flip()
            self.eventHandler(self.events)
            if self.running:
                self.running()

        return self.ret
\$\endgroup\$
  • \$\begingroup\$ Why not just let the code speak for itself? Instead of a long explanation of the design, put the code to be reviewed in the question. (You've included some code, but not nearly enough to constitute a Code Review question, I think.) \$\endgroup\$ – 200_success May 15 '14 at 9:04
  • \$\begingroup\$ @200_successFixed the post, contains integral parts of the code, as well as a link to pastebin (where I can't edit it.) \$\endgroup\$ – asdfghikku May 15 '14 at 9:14
  • \$\begingroup\$ @200_success The reason for the "long explanation" is that I thought my code had regrettably few comments. "But why didn't you just add more comments to the source then?" I honestly don't know, but the explanation is there now. \$\endgroup\$ – asdfghikku May 15 '14 at 9:47
2
\$\begingroup\$

I have a few comments on your OOP design.

Your Tetromino class is way too full of functionality, or in other words, bloated. On one hand, it stores the basic info of location, type etc, but at the same time it is responsible for events, drawing, and collision detection. That is a violation of the Single Responsibility Principle which states that each component should be responsible for 1 task.

Therefore, the Tetromino class should just have a list of the 4 coordinates, and a color. We then subclass Tetromino for all the types of tetrominoes: SquareTetromino, LineTetromino, and etc. which will have convenient initializers for generating the correct coordinates given a single coordinate (such as the upper left corner).

Then, we add a TetrominoController which handles moving the Tetrominos as needed and so has a list of the tetrominos and the dimensions of the board. This class does the bound checking.

Lastly we should have a TetrominoRenderer that interfaces with the TetrominoController to draw the board (the controller will notify the renderer when positions change and so on).

The Game class should interact with the TetrominoController to dictate updates, and it should initialize the renderer (but not do anything else, as the renderer should get notifications from the controller).

\$\endgroup\$
  • \$\begingroup\$ The reason why the game lacks classes for each kind of tetromino, is because the handling of the tetrominos should be as general as possible so that users could add new tetrominos without editing source code, this functionality is currently implemented (though it wasn't when this question was posted.) Of course, since python is a dynamic language it would probably be possible to handle something like this during runtime, but I'm not quite sure how I would handle it, or whether or not there is any reason to do it. Other than the ability to easily distinguish types using type(tetromino). \$\endgroup\$ – asdfghikku Aug 22 '14 at 19:27
  • \$\begingroup\$ I've thought about adding something like an interrupt queue inside of the Game class to handle things like insertion of Tetrominos, which would "de-bloat" the Tetromino class. I would be a bunch of classmethods inside a TetrominoController class, these would then be registered inside of the Game instance, the class is just to organize things a bit so that they aren't global functions. The Job-derived classes (A Job class has been written) would then be able to raise interrupts by adding some sort of a hashable key to the queue, which the Game instance would then look up in its table. \$\endgroup\$ – asdfghikku Aug 22 '14 at 19:40
  • \$\begingroup\$ I'll also add that the design of the program has changed somewhat since I posted this question on StackExchange (so has the user experience, but that's unrelated to the OOP question.) \$\endgroup\$ – asdfghikku Aug 22 '14 at 19:42

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.