# What are the bad habits etc. in this code?

I know this code is pretty awful, but, by any chance, could someone point out all the flaws you can find and tell me them? I think it will help me become a better coder.

``````# Alien Assualt
# A bullet hell alien invasion game.
# Bobby Clarke

#Imports
import pygame, sys, os, random, math, easygui
from pygame.locals import *
pygame.init()

screen = pygame.display.set_mode((600, 450))
pygame.display.set_caption("Alien Assualt")

clock = pygame.time.Clock()

# Glitchy Lazer Mode
#pygame.key.set_repeat(1, 1)

def adv_range(limit1, limit2 = None, increment = 1.):
"""
Range function that accepts floats (and integers).

Usage:
adv_range(10, increment = 0.5)

The returned value is an iterator.  Use list(adv_range) for a list.
"""

if limit2 is None:
limit2, limit1 = limit1, 0.
else:
limit1 = float(limit1)

count = int(math.ceil(limit2 - limit1)/increment)
return (limit1 + n*increment for n in range(count))

def setup(num_of_enemies, enemyclass):
enemies = []
for i in range(0, num_of_enemies):
distance = i * round(400 / num_of_enemies)
direction = random.choice((True, False)) #True is right, left is false
enemies.append(enemyclass(x = random.randint(100, 440), y = distance , right = direction))

return enemies

class player():
"""The player"""
def __init__(self, x = screen.get_width() / 2 -  35, y = screen.get_height() - 45, img = "Images/player.png", speed = 3):
self.x = x
self.y = y
self.rightkeys = (K_RIGHT, K_d, K_KP6) #Keys making it move left
self.leftkeys = (K_LEFT, K_a, K_KP4) #Keys making it move right
self.left = False
self.right = False
self.speed = speed

def move(self):
if self.right and self.x <= screen.get_width() - 60:
self.x += self.speed # Move right
if self.left and self.x >= 0:
self.x -= self.speed # Move left

def fire(self, shot, shots):
shots.append(shot(self))
return shots

class shot():
def __init__(self, player, enemy = False):
self.x = player.x + 25
self.y = player.y

if enemy:
img = "Images/enemyshot.png"
else:
img = "Images/shot.png"

self.enemy = enemy
self.delete = False

def move(self, speed = 5):
if self.enemy:
self.y += speed
else:
self.y -= speed

if self.y <= 0 or self.y > screen.get_height():
self.delete = True

class enemy():
def __init__(self, x = random.randint(0, screen.get_width() - 30), y = 0, img = "Images/enemy.png", left = True, right = False, speed = random.randint(4, 7)):
self.x = x
self.y = y
self.speed = speed

if right:
left = False

self.left =  left
self.right = right

def move(self):
if self.left:
self.x -= self.speed
elif self.right:
self.x += self.speed
if self.x < 0:
self.right = True
self.left = False
elif self.x > screen.get_width() - 60:
self.right = False
self.left = True

def fire(self, shot, shots):
shots.append(shot(self, enemy = True))
return shots

def play(player, enemies, shotclass, secs):
shots = []
key = None
playing = True

while playing:
clock.tick(60)
screen.fill((0, 0, 0)) # Black

player.move()

for shot1 in shots:
shot1.move()

for shot2 in shots:
if shot1.x in adv_range(shot2.x, shot2.x + 10) and shot1.y in adv_range(shot2.y, shot2.y + 10) or shot2.x in adv_range(shot1.x, shot1.x + 10) and shot2.y in adv_range(shot1.y, shot1.y + 10):
if not shot1 == shot2 and(shot1.enemy == True and shot2.enemy == False) or (shot1.enemy == False and shot2.enemy == True):
shot1.delete = True
shot2.delete = True

#Blits

screen.blit(player.img, (player.x, player.y))

for shot in shots:
for enemy in enemies:
if shot.x in adv_range(enemy.x, enemy.x + 60) and shot.y in adv_range(enemy.y, enemy.y + 20) and not shot.enemy:
enemies.remove(enemy)
try:
shots.remove(shot)
except:
pass

if shot.enemy and shot.x in adv_range(player.x, player.x + 60) and shot.y in adv_range(player.y, player.y + 45):
return False, secs

if shot.delete:
try:
shots.remove(shot)
except:
pass
screen.blit(shot.img, (shot.x, shot.y))

for enemy in enemies:
screen.blit(enemy.img, (enemy.x, enemy.y))
enemy.move()
if random.randint(1, 10 * len(enemies)) == 1:
shots = enemy.fire(shotclass, shots)

#   #   #   #   #

pygame.display.update()

if not enemies:
return True, secs

for event in pygame.event.get():
if event.type == pygame.QUIT:
playing = False

elif event.type == KEYDOWN:
if event.key in player.rightkeys:
player.right = True
elif event.key in player.leftkeys:
player.left = True
elif event.key == K_SPACE:
shots = player.fire(shotclass, shots)

elif event.type == KEYUP:
if event.key in player.rightkeys:
player.right = False
elif event.key in player.leftkeys:
player.left = False

elif event.type == USEREVENT:
secs += 1

def main(playerclass, enemyclass, shotclass):
win = False
enemynum = 0
deaths = 0
secs = 0

while enemynum not in range(5, 21):
enemynum = easygui.enterbox("How many enemies do you want? (5 - 20)", "Alien Assualt")

try:
enemynum = int(enemynum)
except ValueError:
enemynum = 0
except TypeError:
sys.exit()

pygame.time.set_timer(USEREVENT, 1000) #Fire a user event every second, for the timer

while not win:
win, secs = play(playerclass(), setup(enemynum, enemyclass), shotclass, secs)
deaths += 1

if secs < 60:
time = "{0} seconds".format(secs)
else:
time = "{0} minute(s) and {1} second(s)".format(math.floor(secs / 60), secs % 60)

easygui.msgbox("You won in {0} and died {1} times.".format(time, deaths))

if __name__ == "__main__":
main(player, enemy, shot)
``````

It's python 3.2.3, by the way.

-
I'm pretty sure I mentioned that using a pastebin here is a bad practice by itself :P. –  Michał Górny Aug 24 '12 at 15:21
A tiny note, you can drop empty brackets when defining a class `class player:` –  GP89 Aug 28 '12 at 17:44

Some problems:

• Classes uncapitalised
• Default parameter equal signs without space (def foo(a=0))
• Too long lines (you can break them!)
• Built-in paths ("Images/player.png")
• Use of too many arguments (use **kwargs instead)
• Return value instead of in-place operations on lists

``````def fire(self, shot, shots):
#You are performing this operation on the mutable object you get
shots.append(shot(self, enemy = True))
#Therefore the next line is unnecessary
return shots
``````

You code is actually not that bad, you just have to read PEP8 and use pylint or something :)

-
What should I use as an alternative to built-in paths? –  Bobby Clarke Aug 24 '12 at 17:31
os.path.join('Images', 'player.png') is cross-platform :) –  pythonm Aug 26 '12 at 16:11

I'm not sure the logic of `adv_range` is doing what you intended:

``````>>> list(adv_range(4.0, 5.0, 0.3))
[4.0, 4.3, 4.6]
>>> list(adv_range(4.0, 5.1, 0.3))
[4.0, 4.3, 4.6, 4.9, 5.2, 5.5]
``````

Normally you would expect a function like this to consistently stop one step before it reaches `limit2`, or one step after. You should specify in the function's documentation exactly which numbers you expect it to return and test that it does this (you could even throw in some doctests). Note that the line `limit1 = float(limit1)` won't have any effect unless `float1` was some unusual type like `Decimal`.

You could also look at numpy if you are going to do a lot of numerical work - it has functions like `numpy.arange` and `numpy.linspace` that do essentially what `adv_range` does.

I haven't looked at this in detail but the way you keep storing directions as a pair of bools (left and right) looks a bit awkward. If you just store the direction as an integer, e.g., +1 for right, -1 for left, 0 for stationary, it could simplify things - you could do `self.x += self.speed * self.direction` instead of testing both `self.left` and `self.right`.

-