Alien battle code

I've recently written a program for Python 3 that I'm using to practice my newly-acquired object-oriented knowledge. It's a simple text game that is really easy to play and can be run straight out of IDLE with no modules (except the Random module). I realize that beginner code can sometimes be sloppy and difficult to read, so lately I've really been trying to improve the beauty in my code. I'd love advice on my code as well as any bugs you may have spotted. Please be as honest (brutally honest, if necessary) as possible, here.

```
# Alien Battle
# 3/26/13

import random

# player class
class Player(object):
health = 20
damage_points = 8
cash = 0
# initial
def __init__(self):
print("You jump out of the window of your crashed ship. "\
"Your fellow soldiers scramble behind cover. " \
"The aliens are coming!")

@staticmethod
def damage_upgrade(points):
Player.damage_points += points
print("Your weapon has been upgraded! Your weapon now does " + str(Player.damage_points) + "damage!")

@staticmethod
def get_cash(cash):
Player.cash += cash
print("Your new balance is $" + str(Player.cash) + "!")

@staticmethod
def get_health(health_pack):
if Player.health >= 20:
Player.health == 20
print("Your health is at maximum, so you give the health pack to a wounded soldier instead.")
else:
Player.health += health_pack
print("Your health is now " + str(Player.health) + "!")

def __str__(self):
stat = "||\\ Player Info /|| \n"
stat += "Health: " + str(Player.health) + "| MAX HEALTH: 20\n"
stat += "Damage: " + str(Player.damage_points) + " | MAX DAMAGE: 20\n"
stat += "Money: $" + str(Player.cash) + " | MAX CASH: Unlimited"
return stat

def check_for_death(self):
if Player.health = 20:
Player.damage_points = 20
print("Your weapon is fully u

I would use Player.health, .damage_points, and .cash as instance attributes, rather than class attributes:

class Player(object):
    def __init__(self):
        self.health = 20
        self.damage_points = 8
        self.cash = 0

A class attribute describes (and applies to) every possible instance of the class, and although there's only going to be a single Player in a given game, this information really is just describing that one player. For example, what if you wanted to add NPC allies? You might want to use the same Player object, but then they'd all have the same health, the same cash, etc.

You already create Player1, so you can just keep that instance through the game loop and change calls to Player to that. Though I'd rename it to player1 -- capitalized names are almost universally expected to refer to class names in Python, not instances of a class. Check out the Python style guide for some general standards.

Doing this, I believe you'd also want to change the staticmethods to just normal methods.

I wouldn't have the Player methods do all the message printing. This might just be my preference, but if I were expanding or debugging this code, I wouldn't expect store's upgrade messages to be relayed to the user through here. What if, for example, you want to add several different stores with different upgrade messages? Or let people find magic assault rifles? And you're already taking that approach with the health upgrades.

Don't use Player's __str__ method this way: it's meant to create a human-readable representation of your object. Just call it .stat, and let __str__ return the player's name or something.

Generally, think very carefully about when something should be a method vs. a function vs. just some inline code. Why is check_damage a function while check_for_death is a Player method? Always ask yourself: In your mental model of the process, what work is being done, and what object is actually doing it? And should it be that way?

You should also look at how you name and implement the Aliens class. Do these represent individual mobs the PC is fighting, as the name implies? Or an encounter and series of events, as its coded? It's really important to use intuitive, descriptive names for your objects: it makes a big difference later when you or someone else returns to your code, and has to make assumptions based on them. If you want to think of an encounter as an object, go all the way: don't just say 'here are three attack methods', but define some possible ways each round of fighting could go, and say 'while we still have alien to kill, do another round of fighting'.

Backing up a bit, your main game routine isn't doing what you might expect it to. This is getting into stuff that's too complex to really go into here, but look at how control is being passed around: you have a whole lot of nested function calls that never actually terminate, and end up just piling up. Let's say someone starts, checks his stats, goes to the store, then makes a move with a randmove value of 8. That's just three actions and he ends up back at the Gameloop menu, right?

Gameloop() # he starts, checks his stats, which calls a new
--> Gameloop() # then he goes to the store, which calls a new
    --> store() # exits the store, which calls a new
        --> Gameloop() # makes a move, which calls a new
            --> Moveloop() # and the randmove value ends up calling a new
                --> Gameloop()

So at this point, you're nested several frames deep in functions that aren't doing anything. At best this is a waste of memory, but think about what happens when one of those frames is created within a while loop that hasn't reached a defined break condition yet.

What you really need to do is have a single main loop, usually a while, that iterates until the player wins, loses, quits, or whatever, and a way of keeping track of the game state -- where the player is, what he's doing, what's going on. As the player does something, control is passed off to new procedures, but you don't call the main loop to return to it. You just let the procedure terminate and return there automatically, because you never actually left it.

A few smaller details, because this is getting long:

Look into string formatting to make your output strings easier to read. Instead of

print("Your new balance is $" + str(Player.cash) + "!")

you could do

print("Your new balance is ${}!".format(self.cash))

(Be sure to read the examples in that link, you'll get some ideas.)

An alternative to the check_health and check_damage functions: take a look at min() and max().

Instead of if randmove == 12 and ... and ..., Python has a nice, concise syntax you can use:

if 12 <= randmove <= 14:

StackExchange Code Review Q#24445, answer score: 7