In this blog post, I’m taking a game off of Pygame.org and going through it to make it more readable and extend its functionality. This is an intermediate level tutorial and assumes some familiarity with Python. This can be pretty helpful if you know programming basics but want to know, “How can I write better code?" (And because someone always brings it up, I have this disclaimer: Of course, these changes are my own subjective idea of "better" and not necessarily changes that another developer would make.)
Demon Kingdom by Logi540 is a defense game where monsters walk from the left side of the screen. The player needs to attack them by clicking on them, and can pick up gems to use for spells. If any monster reaches the right side of the screen, the player loses. Download source code.
The main theme of my changes is:
- Removing duplicate code. Duplicate code is bad because if you have to change it (to add features or fix bugs) you may forget to change it in every duplicated place.
- Remove magic numbers. Magic numbers are hard-coded integer values which are bad because they don't describe what they represent. A better alternative is to use a constant instead (these are the ALL-CAPS variables).
- Put sequential variables in lists and use loops. Instead of having variables named like
spam3, and so on, it's better to have a single list variable, and have a loop run code on each item in the list. This usually leads to a decrease in duplicate code.
- Get rid of unneeded variables. Removing code reaps tons of benefits: there's less code a programmer has to read and understand, less code that a programmer has to look through when debugging, and less code (usually) means less bugs and "moving parts" that could break.
Here's the inital check in of all the files. The file I'll be modifying is named
demonkingdom_makeover.py. This program requires Pygame to be installed to run. I recommend downloading the game, playing it a couple times, and looking through the source code before continuing with this article. For each section, open up the diff link to see what the exact changes I made were.
Remove Dependency on easygui
Diff of these changes: Removed easygui requirement and changed image & sound file paths
The first change I'm going to make is to get rid of the use of the
easygui module. The game uses this just to display a message box that displays some credits. Without this call to
easygui.msgbox(), the fewer additional modules the user will need to run this game (and the more likely people will play it).
I've also put all the image files under their own folder named "images", so I'll have to change the lines of code that load these to include the new path.
Add Spaces for Readability
Diff of these changes: Renamed SpellIcon class. Added spaces after +, -, and * operators.
As a general cleanup of the code to make it more readable, I prefer a single space after commas in the source code so that it is not bunched together. Using the find-and-replace feature in my text editor, I replace all commas with a comma and space. Diff of these changes: Added spaces after all the commas
I do some more cleanup by adding spaces around the
* operators in the source code, as well as capitalizing the
spellIcon class. It is a common convention in programming to have class names begin with a capital letter.
Improve the Background Class
Diff of these changes: Improved the Background class.
Next, I make some improvements to the
Background class. This class loads and stores all the background images as
pygame.Surface objects. Previously the single
Background object would be passed an image object with its
changeBackground() method. In order to put all the background-loading code in the same place, I had this take place in the constructor method of the class.
Background constructor is now passed a list of background images, which is loads and stores in its
imageList member. A single list variable is always preferable to variables like
back4, and so on. Now when
changeBackground() is called, we simply pass it the index of the image it should be changed to.
rect member is updated based on the image when
changeBackground() is called. Previously the code assumed that all the background images were the same size and used the rect from the first background for all the backgrounds.
Replace Magic Numbers with Constants
Diff of these changes: Improved Sidebar class. Created numGems global. Added size constants. Renamed Logo class.
Next I want to get rid of the magic numbers used in the source code. These a numbers that appear in the source code that do not have explanations, which can make it hard to figure out what they are used for. Instead we will replace them with constant variables with descriptive names. These include the
I also rename some variables to be more descriptive (such as changing
numGemsCache) and created a new
numGems global variable. Normally I don't advocate using global variables, but in this case the code had placed the
gems member in the
Sidebar class. This is odd because the
Sidebar class should deal with the user interface and displaying game info, not tracking the state of the player. Also, only one
Sidebar object is created, effectively making it a global variable anyway. This change also lets me get rid of the
addGems() method from the
I replace several calls to the
SpellIcon constructor with a loop (lines 34 to 41 of the original file), and capitalize the name of the
Get Rid of the Unneeded load() Method
Originally each monster is an object of the
Monster class, which is a child class of the
MySprite class, which itself is a child of Pygame's
pygame.sprite.Sprite class. This is an odd choice to make because a monster is not a sprite image, but rather several things that can include sprite images. However, I've found it difficult to break out this inheritance structure, so instead I just renamed
MySprite to the more descriptive name
I renamed the color-related variables (
lightGrey, etc.) to capitalized names to show that they are constants.
load() method in the
AnimatedSprite class was always called directly after the constructor, I saw no reason why there should be a separate
load() method and moved its code into the constructor.
first_frame member variable never has a value other than
0, so I got rid of it and simply used
0 in its place. (Though it could be argued that that is a magic number and should be replaced with a constant.)
I capitalized the name of the
Make __str__() More Debug-Friendly
Diff of these changes: Added labels to the __str__() method of AnimatedSprite.
__str__() method of a class is what is called when a string value representation of the object is needed. This is useful to print out debugging information about an object, but the
__str__() method only printed out values with labeling what they are. The developer would have to look back at the source code to find what each number stood for. I've added labels to these values.
Add Spacing to Make MONSTER_STATS More Readable
Diff of these changes: Improved readability of the monster stats, renamed stats constant.
stats global variable holds all the details about the image, size, life, and speed attributes of each type of monster. I renamed this variable to
MONSTER_STATS (to denote it as a constant) and also added some spacing and newlines to make it easier to read in the source code.
Change Lists to Tuples, Make Monster Constructor Call More Compact
Next I renamed the
rect member with the more descriptive
frame_rect name (it is the rect object for the frame of the monster's image. I replaced the lists in the
MONSTER_STATS global with tuples since these values do not change. A tuple is immutable; and using a tuple conveys to other programmers that these values never change when the program runs.
I also call the
Monster constructor using an asterisk with
MONSTER_STATS[type]["image"]. The asterisk passes all of the items in the
MONSTER_STATS[type]["image"] tuple as individual arguments to the
Monster constructor. This is much more compact than calling
Monster(screen, MONSTER_STATS[type]["image"], MONSTER_STATS[type]["image"], MONSTER_STATS[type]["image"], MONSTER_STATS[type]["image"]).
Don't Use Python's Existing Names, Don't Use Sequential Variable Names
Diff of these changes: Put variables into lists.
The use of
time as a name used in different parts of the source code is a poor choice because there is a module already named
time as part of the Python standard library. You shouldn't create your own variables with the names of things that exist as a part of Python, because it can confuse other developers and lead to bugs in your code. (For example, if we also imported the
time module in this program, we might accidentally use the wrong
time at different places in the code.) I renamed it to
current_time. Diff of these changes: Renamed time to current_time.
The source code has several sequential variable names such as
level3Text, and so on. These would be much more manageable in a single list variable. This way we can put the code that uses them into a much more compact loop, rather than a lot of copy/pasted code.
Also, I renamed the
sword variable to
swordSound, which is more descriptive of what it contains and is consistent with the other sound-related variable names.
Shorten "== False" and "== True" Code
Diff of these changes: Shortened the == False and == True expressions.
Instead of having expressions that check for equality with
False, it is more concise to leave this out. I rewrote code like
while introdone == False: to the more simple
while not introdone:.
Put Copy/Pasted Code Into Loops
Diff of these changes: Put spell casting code in a loop.
I rename the
spells variable to
SPELL_STATS to show that it is a constant. (Oddly again, this variable is in the
Sidebar class. I'll move it out in a future change.)
Instead of having a separate
elif block for each of the three spells, I made a loop that goes through the
SPELL_STATS variable and generically handles each spell. This will make it much easier to add spells to the game in the future.
numGems = 90 change was mistakenly checked in. I had set it to this to test out the game.)
Loops are always preferable to copy/pasting code, because if you have to make a change to it later, you only have to change the code in one place instead of every place it was copy/pasted.
Get Rid of Unneeded mouseLastDown Variable
Diff of these changes: let MOUSEBUTTONDOWN event handle clicking code. Got rid of mouseLastDown variable.
Instead of checking the
pygame.mouse.get_pressed() function to see if the mouse button is currently pressed, we can just examine the MOUSEBUTTONDOWN event that gets generated with each mouse click. This lets us get rid of the extra
mouseLastDown variable, and the fewer variables our program has, the easier it is to understand.
Put Monster Ratios into a MONSTER_RATIOS Variable
Diff of these changes: Moved the monster proportion data to a new MONSTER_RATIOS variable.
Each level has a group of randomly selected monsters from code like
type = random.choice(["bat", "bat", "bat", "bat", "plant", "plant", "plant", "orc", "orc", "orc2", "slime"]). The
random.choice() function will randomly pick a monster from the list. Instead of having these values hard-coded across the source code, we can put them all into a single
That's it for Part 1. In the next part, I'll continue to remove duplicate code and make some readability changes.