7
\$\begingroup\$

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 <= 0:
            print("You failed to survive against the alien horde. As you and your soldiers fall, not a single man gets to the ship alive.")
            input("Press the [ENTER] key to exit the program.")
        else:
            GameLoop()


# Enemy class(Alien)
class Aliens(object):
    # initial
    def __init__(self, damage, enemies):
        self.damage = damage
        self.enemies = enemies 
        self.__upgrade_damage(self.damage)

    def __upgrade_damage(self, damage):
        upgrade = random.randrange(1, 8)
        if upgrade == 2:
            self.damage = 7

    # attack methods

    def attack1(self):
        self.next_wave()
        print("The alien leaps at you and begins biting you. Your fellow soldiers pulls it off of you and you shoot it in the head before it can stand back up.")
        Player.health -= self.damage
        Player1.get_cash(2)
        self.enemies -= Player.damage_points

    def attack2(self):
        self.next_wave()
        print("You and two other soldiers blast 3 aliens as they leap at you through a door. When you check the bodies, you find a health pack!")
        Player1.get_cash(3)
        Player1.get_health(2)
        self.enemies -= Player.damage_points * 1.5


    def attack3(self):
        self.next_wave()
        print("A large horde manages to flank you and take our three of your men along with severely hurting you. The medics aren't sure if you'll make it.")
        Player1.get_cash(1)

    def next_wave(self):
        if self.enemies <= 0:
            print("You and your team have wiped out all of the enemies, but you feel another horde coming.")
            self.enemies += 15
            Player1.get_cash(4)
            GameLoop()
        else:
            pass

Ali = Aliens(damage = 5, enemies = 17)
# Move loop
def MoveLoop():
    randmove = random.randrange(1,16)
    if randmove == 1 or randmove == 2 or randmove == 3 or randmove == 4 or randmove == 5 or randmove == 6:
        Ali.attack1()
        Player1.check_for_death()
    elif randmove == 7 or randmove == 8 or randmove == 9:
        print("While on the battlefield, you manage to grab some extra money from a dead soldier while your teammates fight back the horde.")
        Player1.get_cash(2)
        GameLoop()
    elif randmove == 10 or randmove == 11:
        print("While you have some extra time, you scavenge for supplies. You find a small health pack among the wreckage.")
        Player1.get_health(2)
        GameLoop()
    elif randmove == 12 or randmove == 13 or randmove == 14:
        Ali.attack2()
        Player1.check_for_death()
    else:
        Ali.attack3()
        Player1.check_for_death()

# check damage, make sure it doesn't go over 20        
def check_damage():
    if Player.damage_points >= 20:
        Player.damage_points = 20
        print("Your weapon is fully upgraded!")
    else:
        pass

def check_health():
    if Player.health >= 20:
        Player.health == 20
        print("Your health is at the maximum level!")
    else:
        pass

# store function
def store():
    print("\n Store Menu")
    print("Your balance: $" + str(Player.cash))
    print("""
1 - Upgrade Weapon (+1 damage) - $6
2 - Upgrade Weapon (+3 damage) - $10
3 - Buy health (+3 health) - $4
4 - Buy Health (+5 health) - $6
5 - Go back to main menu
"""
          )
    store_choice = input("What would you like to buy?: ")
    while store_choice != "5":
        if store_choice == "1":
            check_damage()
            if Player.damage_points < 20 and Player.cash >= 6:
                Player.damage_upgrade(1)
                store()
            else:
                if Player.damage_points > 20:
                    print("Your damage is already at the maximum level!")
                    store()
                elif Player.cash < 6:
                    print("You do not have enough money to purchase this!")
                    store()
        elif store_choice == "2":
            check_damage()
            if Player.damage_points < 18 and Player.cash >= 10:
                Player.damage_upgrade(3)
                store()
            else:
                if Player.damage_points > 20:
                    print("Your damage is already at the maximum level!")
                    store()
                elif Player.cash < 10:
                    print("You do not have enough money to purchase this!")
                    store()
        elif store_choice == "3":
            if Player.health < 17 and Player.cash >= 4:
                Player.health += 3
                print("Your health has been increased! Your total health is now: " + str(Player.health))
                store()
            else:
                if Player.health > 17:
                    print("Wait for your health to lower before buying this weapon!")
                    store()
                elif Player.cash < 4:
                    print("You do not have enough money to purchase that!")
                    store()
        elif store_choice == "4":
            if Player.health < 15 and Player.cash >= 6:
                Player.health += 5
                print("Your health has been increased! Your total health is now: " + str(Player.health))
                store()
            else:
                if Player.health > 15:
                    print("Wait for your health to lower before buying this weapon!")
                    store()
                elif Player.cash < 6:
                    print("You do not have enough money to purchase that!")
                    store()
    if store_choice == "5":
        print("Sending you back to the Main Menu!")
        GameLoop()
    else:
        print("That is not a valid option! Sending you back to store menu!")
        store()

# Main game loop    
Player1 = Player()
def GameLoop():
    print("\nGame Menu")
    print(
        """
1 - Next Move
2 - Go to store
3 - Get player status
4 - Quit game
"""
        )
    choice = input("Choice: ")
    while choice != "4":
        if choice == "1":
            MoveLoop()
        elif choice == "2":
            store()
        elif choice == "3":
            print(Player1)
            GameLoop()
    if choice == "4":
        input("Press the [ENTER] key to exit the program.")
    else:
        print("That is not a valid option! Sending you back to menu!")

# start the game
GameLoop()
\$\endgroup\$
  • \$\begingroup\$ I think toxotes gave a good explanation and I am agree with him. You should do object oriented programming if you are dealing with classes and all. Ok, I will add just one more point, please try to read this first python.org/dev/peps/pep-0008 it will help you to write more readable code. cheers! \$\endgroup\$ – vivek Mar 28 '13 at 3:56
7
\$\begingroup\$

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:
\$\endgroup\$
  • \$\begingroup\$ Hey, thanks so much for the great answer. All the things you mentioned are gonna help me on my next project. I'm already looking forward to the extra updates that you're gonna provide for me later. Thanks again, I'm taking notes from all of the answers and these are some good things to keep in mind. \$\endgroup\$ – hasherr Mar 28 '13 at 0:21
  • 1
    \$\begingroup\$ I would go a step further and have both Player and Alien subclass from an Entity class that holds things like health, position, death status, etc. \$\endgroup\$ – whereswalden Oct 12 '14 at 21:43

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.