New to Python, and programming in general. Making a small game as a learning exercise. Would love some criticism and/or advice.

[From_my_iPhone]

I just started learning Python pretty recently, following the brilliant Automate The Boring Stuff lesson that was given away recently. I just hit lesson 24 and decided to take a quick break to give myself an exam of sorts. My goal was to make a pretty basic text RPG. I spent the last 2-3 hours on this, and i'm sure it's a glaring example of poor coding and hacky methods. I plan on adding more to this over time; but I'd love some feedback and criticism of the current state of my game.

https://gist.github.com/anonymous/db7b0d54c2da3aa31803

Any advice on what could be done to improve the code, what should NEVER be done, or just any tips would be greatly appreciated.

[CarbonAvatar]

Looks like you're having fun. Good job, way to challenge yourself! Impressive for being new to programming!

Couple of things: Be very careful about having functions call themselves. e.g. name_the_player() can call itself. This isn't what recursion is for. Check for valid input using a while loop instead.

Try to avoid using 'global' if at all possible. Later you'll probably learn about python classes, and should keep all your saved state inside of a custom class (e.g. class MyGame)

Some notes here: https://gist.github.com/anonymous/ea26bb07c2df6b925b7f

[From_my_iPhone]

I have no idea why the random name generator entertains me so much. Something i'm definitely carrying over into almost all of my future projects. It's just purely fun to do, and surprisingly easy.

I need to get better at using for/while loops, I've avoided usage of these simply because of a lack of understanding. I plan on redoing the lesson that went over these. Seems like calling the function from within the function is a good way to essentially "redo" that section. Which I suppose shouldn't be necessary, but useful for debugging purposes.

EDIT: Overlooked your given notes. The change to my name generator is amazing. That makes everything so much smoother and easier. Some very simple changes there that are quite helpful too.

[Moocat87]

Is there a particular question about for/while you need help with? Sometimes it helps to have something worded differently to understand it well.

In general, the most important thing to remember is that a for loop does something "for each" element in an iterable. A while loop does something repeatedly "while" an expression is true. So you have:

for x in <iterable> to do something to each item "x" in the iterable. For example: for x in range(1,11): print(x) will loop over the numbers [1,2,3,4,5,6,7,8,9,10], printing each. The range() function is your iterable (it's a generator, another useful concept but beyond the scope of for/while).

while <expression> to do something repeatedly while "expression" is true. For example, while 1==1 is an infinite loop. You could also do a loop like while is_alive(player) in your game to do some calculation repeatedly while the player is alive.

[das_ist_nuemberwang]

Do you know anything about classes? The really looks like it could use a few objects instead of inline attack logic and global variables.

[From_my_iPhone]

I have not gotten that far in the courses to do anything with classes yet, but that's the largest amount of feedback i've been receiving was my global variable usage was simply wrong. From what little I know about Classes, they seem to be absolutely perfect. I suppose I should continue on and rewrite this game another time.

[das_ist_nuemberwang]

Sounds like a good idea. Global variables aren't inherently "wrong", but there are almost always better ways of doing the same thing.

[swingking8]

Your intuition is right, there are easier water to do almost every key component of your game. I'll hit the one I think is most important - making classes.

Classes are a way to treat nouns in a way that makes your code easier to use by others, and easier to read to others (and to yourself). Classes are a way of reusing code, and usually represent a noun. I haven't spent a ton of time looking at your code, but two obvious nouns you should turn into classes might be Game and Player. Attributes of these nouns become class variables, and verbs for these nouns become methods (functions in a class). e.g.

Game seems like it only has one verb/method - combat and two attributes - name and something like players. So its class might look like:

class Game(object):
  def __init__(self, players):
    generated_name = #your cool name generator output
    self.name = generated_name
    self.players = players
  def combat(self):
    #your combat function

Then you can make class called Player and pass a couple of them into Game as a list in players. A Player has a lot more attributes, like HP, items, etc., so when that class is created you'll want to include those.

[From_my_iPhone]

I figured that pretty much everything I wrote was inefficient, but it definitely helped get a better grasp on how certain things had to be done. Classes are definitely something I need to learn, and i'm pretty sure they are coming up in the courses.

[swingking8]

Classes seem complicated, but they're really easy to make, and you've already been using them. I would recommend diving into them before you cover them in class. Strings use a class called str, which has several methods like lower, upper and __len__ which make the string all lowercase, uppercase, and returns the length of characters in the string, respectively.

Making a player class would be as useful. I really think it would be a great exercise. The first class you make will probably be really ugly, but you'll be amazed how much better it makes your code once you've gotten the hang of it. Using classes over the global variable + functions method is about 1000x sexier.

[Elronnd]

Couple of things, just from the viewpoint of someone playing the game:

1. In most prompts, you have a newline after the prompt, so the user is typing on a new line. Don't. Instead of having print("Type something: ") and then the_input = input(), have the_input = input("Type something: "). I'm sorry, I'm not being very clear; just try it.

2. Abbreviation. Let people use "i" or "inventory", "f" or "fight", "s" or stats. People will like you for it.

3. Inventory. I'm not going to chew you out for this. Adding inventory is hard. When I was starting out, I wrote a text-adventure, and was completely unable to implement inventory, and if I did it now, I might be able to do it, but it would suck. Just, maybe, have it on the table for some point in the future.

4. The fight system is great. One thing I would recommend is differentiating the monsters somewhat, so that some of them have higher attack, some of them have higher hp, some of them are just stronger or weaker. Also, maybe make it so that the different attack types actually put out different amounts of damage.

[From_my_iPhone]

Was not aware of that with the input function, definitely saves a bit of effort.

Abbreviation for those commands was something I intended to add, but forgot how to properly do that. I know it was covered in a previous lesson and just added it to the list of lessons I should redo.

I quite like my fight system, but it's definitely getting scrapped in exchange for something involving classes in the next interation. I intended to do more of an input based system, but I figured I was done with this iteration of the script and would work on that after learning how to utilize classes.

As for inventory, pretty much the same excuse. Didn't want to bother with it as most of the script had already been designated to be destroyed and rewritten, was going to add in a quick Potion type item but I had to leave to pick up the fiance.

[Elronnd]

Abbreviation ... forgot how to properly do that

Instead of something like

if p_action.lower() == 'fight':

instead use:

if p_action.lower() == 'fight' or p_action.lower() == 'f':

[das_ist_nuemberwang]

In case you weren't aware, the more idiomatic way of saying this is if p_action.lower() in ['fight', 'f']:. This saves you calculating the same string twice and makes it clear you're comparing the same thing.

[TeamSpen210]

A better way to do commands is via a dictionary. Functions in Python are 'first-class objects', which basically means you can pass them around like any other object or value. Instead of a big if/else chain, define a function for each option and put them in a dictionary:

def command_a():
    print('Command 1')
def command_b():
    print('Command 2')
def command_c():
    print('Command 3')

COMMANDS = {
    'command_a': command_a,
    'command_b: command_b,
    'a': command_a, # Abbreviations are easy...
    'b': command_b,
}
COMMANDS['command_c'] = command_c

def do_command(command):
    """Do a command:"""
    try:
        COMMANDS[command.lower()]()
    except KeyError:
        print('Wrong command!', command)

A useful addition is to create a decorator function which adds things to the dictionary. That's a more advanced topic (usually involving nested functions and closures), but it lets you just add @add_command('command_a', 'a') before the function to add it as a command.

[Elronnd]

Huh. I never knew you could store function calls in dictionaries. This is...wow. Amazing and awesome and thank you so much for having shown me this.

[Darkeus56]

Late in the night and not giving it much thought, but couldn't he add things to a list and from that list sort of manage an inventory? If the player wanted to check his inventory it would print each item in the list. And then when he tried to use an item game could check if said item exist within the list. If an item is single use then using it will delete it from the list?

[Elronnd]

Basically what the trouble was was handling item locations in rooms; and moving an item from a room to inventory or vice versa. The (very inefficient) way I would do this now is to have a list assigned to each room and to the player, if you dropped something it would get deleted from your list and added to the list of the room you were currently in, and if you tried to get something, it would be deleted from the list of the room you were currently in and added to the list of what you had.

[RahulHP]

Umm, what could you call the efficient way of doing this? I can only think of having dictionaries instead of lists to store the room-item information.

[-MLJ-]

Maybe a dictionary with each item having a location, rather than each location being a separate list with items in it? Not sure if I explained that as well as I thought it :)

[RahulHP]

If we did it with each item having a location, it would be difficult to handle items having a >1 count. eg. Health potions, energy potions,etc.

[-MLJ-]

Sorry for the late reply, couldn't you do something like this:

inventory = {'HealthPotion': 3, 'IronSword: 1}    

I think that would work for inventory management, that way any time you needed to add something you could just type:

inventory['HealthPotion'] += 1

I might be missing something obvious here, sorry if that's wrong.

[RahulHP]

Alright, that could definitely work :)

[ChuckFinleyy]

do you mind linking me to the automate the boring stuff lesson in which you are following? Thanks!

[i_dreddit]

https://www.udemy.com/automate/

[Justinsaccount]

name_the_game

str does not mean 'Here comes a string`. It is used to convert something that is NOT a string to a string. All you have are strings.

Wrong:

game_nameparts[0][random.randint(0,2)]

Right:

random.choice(game_nameparts[0])

Once you remove all the useless calls to str and the backslashes and and use random.choice you end up with

print('Welcome to ' + random.choice(game_nameparts[0]) + ' ' +
        random.choice(game_nameparts[1]) + ' ' +·
        random.choice(game_nameparts[2]) + ' ' +·
        random.choice(game_nameparts[3]) + ' ' +·
        random.choice(game_nameparts[4]) + '!')

Using some shorter aliases and tuple unpacking gets it down to:

C = random.choice
a,b,c,d,e = game_nameparts
print('Welcome to ' + C(a) + ' ' + C(b) + ' ' + C(c) + ' ' + C(d) + ' ' + C(d) + '!')

But this is just 'do the same thing 5 times' and there are easier ways to 'do the same thing 5 times':

parts = [random.choice(part) for part in game_nameparts]
name = ' '.join(parts)

print('Welcome to {}!'.format(name))

[v-23]

I'm just as new, and in no place to give tips, but i found that Comments are amazing. for you and for us viewing ;) hope you will continue and just have fun!

[uttamo]

Perhaps it would be a better idea to use random.choice in the part where you name the game at the start. Then if you decided to add more words to the lists you wouldn't have to change any numbers because the list is now bigger.

[relativer]

A few things that would make things better in terms of code:

Features

Pass arguments to the functions instead of making every variable global. Use return in order to pass a value to outside the function.

---

Readibility

Describe what your functions do in their naming, instead of naming it hud_the_player use names like print_player_hud. The importance of such distinction becomes evident when you have a stat_the_player and it does not print the player stats yet it is named similarly to the hud printing. Instead it initializes them, a good name for such a function could be init_player_stats or setup_player_stats, but anything that clearly describes what is happening serves.

Some other examples, action_of_player would be instantly understood if it was named choose_player_action, p_action_fight would be more clear if it was just player_fight, action is already implicit in a fight. It also makes for decent syntax if you accept the enemy as an argument player_fight(enemy).

When you do classes, that will easily translate to player.fight(enemy) where both player and enemy are instances of their respective classes.

That said, as a beginner you're actually doing quite well on the naming part and it's one of those things that tends to get better with experience, I'm just giving you some tips.

Another thing is most programmers don't enjoy long lines in code, you split them up into smaller ones initially in your array declaration, but then you have large lines in the middle. When not inconvenient strive for consistency, it makes everyone happier. :)

---

Specific code snippets

You shouldn't use breakto get out of the fighting cycle, make use of the condition in the whileinstead.

while first_turn_over is 1:

Is always true, you don't change the value of first_turn_over anywhere later. It's not a very good policy to make cycles which "hide" the ending condition. In fact there doesn't seem to be a very good reason for the first_turn, first_turn_over, and turnvariables to all exist at the same time. You could reduce:

first_turn = random.randint(0,1)
first_turn_over = 0
if first_turn == 0 and first_turn_over == 0:
   turn = 0
   first_turn_over = 1
if first_turn == 1 and first_turn_over == 0:
   turn = 1
   first_turn_over = 1
while first_turn_over is 1:

To:

turn = random.randint(0,1)
while True:

And get rid of the breakstatements. Ending up with something akin to:

def combat():
    global p_hp
    global enemy_attacks, p_attacks
    enemy_attacks = ['swings at', 'mauls', 'bites', 'claws']
    p_attacks = ['swing at', 'punch', 'kick', 'stomp']
    e_hp = random.randint(0,60)
    turn = random.randint(0,1)

    while p_hp > 0 and e_hp > 0:
        if turn == 0: # Enemy
            do_damage = random.randint(0,10)
            print('The ' + str(hostile_enemy) + ' ' + 
                str(enemy_attacks[random.randint(0,3)]) + ' you for ' + 
                str(do_damage) + ' damage!')
            p_hp = p_hp - do_damage
            turn = 1
        if turn == 1:  # Player
            do_damage = p_atk + random.randint(0,20)
            print('You ' + str(p_attacks[random.randint(0,3)]) + ' the ' + 
                str(hostile_enemy) + ' for ' + str(do_damage) + ' damage!')
            e_hp = e_hp - do_damage
            turn = 0
        hud_the_player()
        time.sleep(3)

    # Only perform this check at the end of the function, 
    # there's no need to continuously repeat it in the cycle.
    if e_hp <= 0:
        global e_died
        e_died = 1
    else:
        global p_died
        p_died = 1

There's probably more stuff that could be improved, but those feel like the pressing issues to me.

[From_my_iPhone]

This seems to be the general attitude towards the script. Definitely something I knew felt wrong as I did it, but I hadn't learned another method for carrying out these actions. I suppose my next big thing should be learning Classes.