I'm making a GUI system in Python for game development with PyGame. Right now I have a button that changes colors when you hover and click on it. My question is, is this code well designed or can it be improved?


import sys, pygame
from pygame.locals import *
from gui import *

def input(events):
    for e in events:
        if e.type == QUIT:

pygame.display.set_caption("GUI Demo")

window = pygame.display.set_mode((640, 480))
screen = pygame.display.get_surface()
clock = pygame.time.Clock()
font = pygame.font.Font(None, 32)
button = Button("btn_1", "Hello World", 50, 50, 200, 120)

button.decorate((255, 0, 0), (0, 255, 0), (0, 0, 255))

while True:
    screen.fill((120, 40, 200))
    button.update(pygame.mouse.get_pos(), pygame.mouse.get_pressed())


# Mini-GUI demo

import pygame


class Surface: 
    def __init__(self, x, y, w, h, color):
        self.x = x
        self.y = y
        self.w = w
        self.h = h
        self.color = color

    def collides(self, surface):
        return self.y < surface.top + surface.height and self.y + self.h > surface.top and self.x < surface.left + surface.width and self.x + self.w > surface.left

    def getCoords(self):
        return (self.x, self.y)

    def draw(self, surface):

class GUIElement(Surface):
    def __init__(self, id, x, y, w, h, color):
        super(GUIElement, self).__init__(x, y, w, h, color)
        self.id = id

class Textbox(GUIElement):
    def __init__(self, id, x, y, w, h, color):
        super(Textbox, self).__init__(id, x, y, w, h, color)
        self.data = ""

    # TODO: update and draw

class Button(GUIElement):
    def __init__(self, id, text, x, y, w, h):
        super(Button, self).__init__(id, x, y, w, h, None)
        self.text = text
        self.font = pygame.font.Font(None, 32)
        self.state = BUTTON_NORMAL
        self.normalColor = None
        self.hoverColor = None
        self.activeColor = None

    def decorate(self, normalColor, hoverColor, activeColor):
        self.normalColor = normalColor
        self.hoverColor = hoverColor
        self.activeColor = activeColor
        self.color = self.normalColor

    def update(self, mouseCoords, mousePressed):
        (up, down, middle) = mousePressed
        if len(mouseCoords) < 4: mouseCoords = pygame.Rect(mouseCoords[0], mouseCoords[1], 1, 1)

        if not self.collides(mouseCoords): 
            self.state = BUTTON_NORMAL
            self.color = self.normalColor

        if up:
            self.state = BUTTON_ACTIVE
            self.color = self.activeColor
            self.state = BUTTON_HOVER
            self.color = self.hoverColor

    def draw(self, surface):
        pygame.draw.rect(surface, self.color, pygame.Rect(self.x, self.y, self.w, self.h))
        surface.blit(self.font.render(self.text, 1, (255, 255, 255)), self.getCoords())
  • \$\begingroup\$ The comment "TODO: update and draw" suggests that this code might not be ready for review yet. It's best for you to fix all the problems you know about before submitting it for review. \$\endgroup\$ Mar 23, 2013 at 22:30
  • \$\begingroup\$ That's just for the textbox element. The button is complete so far. \$\endgroup\$
    – Ryan
    Mar 23, 2013 at 23:55

1 Answer 1

  1. There are no docstrings. What is the purpose of each of your classes and methods, and how am I supposed to call them?

  2. If you click outside the button, and then slide the pointer over the button, it becomes active. Normal GUI buttons require the initial click to be on the button in order to be able to activate it.

  3. The class name Surface runs the risk of confusion with pygame.Surface. Or in some applications, of shadowing it.

  4. The GUIElement class is a Surface with an id. But none of the code uses the id, so this class is useless. (No doubt you have some purpose in mind, but what is it?)

  5. Calling sys.exit() makes it awkward to test your program from the interactive interpreter.

  6. If you care about portability to Python 2, you should make Surface inherit from object so that it is a new-style class. On the other hand, if you don't care about portability to Python 2, just call super() instead of super(GUIElement, self) or whatever.

  7. Surface takes arguments x, y, w, h. Why not use a pygame.Rect? Then you could just call Rect.colliderect instead of writing your own version.

  8. Surface.draw does nothing. Is this because Surface is an abstract base class? If so, you should use metaclass = ABCMeta and decorate the draw method with @abstractmethod to make this clear.

  9. In order to create a coloured button, you have to create the Button object and then call its decorate method to set its colours. Why not allow the caller to set the colours when they create the button? (Via optional keyword arguments.) Similarly for the font.

  10. Since you can deduce a button's color from its state, you don't need a separate color attribute (or do you?). By having two attributes you run the risk of them getting out of sync. Why not have a map from state to colour?

  11. up, down, middle are misleading variable names: the values returned by pygame.mouse.get_pressed() are actually the state of the three mouse buttons. Use names like button1, button2, button3.

  12. pygame.mouse.get_pos() returns the mouse coordinates (x, y) so it's not clear why you write if len(mouseCoords) < 4. Won't this always be true? And why do you need to make a rectangle here anyway?


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.