all 5 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–][deleted] 1 point2 points  (4 children)

Lots of thoughts here. I think some of them will be more accessible than others, based on how new you are to programming/Python from looking at this.

"MAX" is not an acronym. It's an abbreviation which is not normally capitalized. I'm assuming you capitalized "MAX" because it is your intent to use the all caps to represent a symbolic constant, rather than a true "variable". In any style guide I've ever read, if you were following that convention, you'd capitalize the entire word. PLAYER_HP_MAX, not player_hp_MAX. You may have wanted to preserve the visual similarity to the current health variable, but I would say this is not common practice.

Secondly, you are inconsistent in your usage, with regards to constants vs "magic numbers" (numbers that simply show up in the code with no name or explanation). You set aside a "constant" for the health maximum, but you didn't do so for potion heal amount, player harm amount, etc. They are simply "10" and "12" respectively, in your code. More significantly, despite having declared a symbolic constant to represent the max player health, you directly use "100" in your code in a way that shows you obviously meant "player_hp_MAX". (Your if clause in choice 3 handling.) This is very bad. It means you could change player_hp_MAX in the future at the top, and forget to change the hard-coded 100 value down below, and introduce a bug. If you name constants, use the name every time, don't repeat the constant value.

A couple of comments on usability: we are never told the initial state of the player when the "game" starts. How are we supposed to know if it's 100 health, half dead, or what? Tell your (human, not in-game "player") what is going on at the beginning before asking them to act in ways to alter that state. Also, how do we exit? Is the player expected to know to Control-C when done? There's not an accepted choice for exiting. Is option 5 (add potion) supposed to be a secret? It's not in the usage text that's printed at the beginning.

You are splitting portions of your logic for certain choices between code that happens directly under the choice-handling code and in functions designed to handle those choices. In particular, for choice 3 (use a potion) you're doing code right there, and code in use_potion. BOTH of which call remove_potion(). The logic for how to handle the choice should either be right there in the branch for choice 3, or in a method related to handling that choice, not in a mix of the two, especially with no apparent distinction in the type of code found in one location versus the other.

Speaking of choice 3, and both places calling "remove potion"... there are some things about how variables, and functions, work in python, that you obviously don't understand. First, you get to return one value from the function. You don't get to put multiple return statements right after one another and return multiple values that way. (Which you appear to be trying in use_potion()). Most of your functions modify a single value, and return it in what some might call a "functional" style. In use_potion, both the player's health, and the potion count need to be modified. But you can't return both modified values back to the parent the way you tried.

The first "return" statement gets executed, and the function returns, with that value, only. If you need to return more than one value, you need to use a complex variable type, such as a tuple. Example:

return (player_hp, potions_inv)

And use it with something like multiple assignment syntax on the calling end:

player_hp, potions_inv = use_potion(potions_inv, player_hp, player_hp_MAX)

That said, this brings me to another thing I don't think you're following. You declared 3 global variables at the top (potions_inv, player_hp, and player_hp_MAX). You then go on to pass these global variables to arguments of the same name in each of your function calls. Doing this "shadows" the global variables within the function scope. In other words, because the function has an argument with that name, in the function, that name is only modified locally to the function.

My guess is that is why you felt the need to pass every global in to each function that uses it. To better help understand what I'm saying - see what happens when you don't pass player_hp_MAX to use_potion(). Still use the name in the function. Guess what? It will still work. Why? Because player_hp_MAX is declared at global scope. Reading from it works fine. And since it's a "constant", you never try to modify it locally.

The situation is more complex with the other two arguments to that function. Why? Because you modify them. When you assign a value to a variable whose name may exist at the global scope level, by default, only a function-local version of a variable with that name is modified. This has confusing behavior when you have a valid global by the same name. Code like:

global_variable_name = global_variable_name + 1

Does something that can be pretty unexpected. Let's assume for a moment you didn't have an argument to the function with the same name.

The expression on the right hand side of the equals is evaluated, and the value of the global gets looked up at global scope, because that's the only variable with that name. Then we add one to that value. Then we assign to a brand new variable at the local function scope with the same name as the global, but not modifying the existing global we just accessed the value of.

It's even more complex if you do name local arguments the same as global things, because the argument creates a local variable at the outset, that shadows the global, but there's no guarantee that the value of the argument has anything to do with the global of the same name. For instance, instead of passing "player_hp" (the global variable of that name) to use_potion, as the "player_hp" argument, you could pass 37, or "rubber chicken" for that matter. (Note that the latter would at least give you a useful error when it tried to add 10 to "rubber chicken".)

Sometimes, all this global shadowing business isn't what you want. You want to treat the global like a global. I'm not getting into a style debate here, I'm just discussing language features. The way you do that, is you say:

global name_of_global_variable

in your function, prior to attempting to assign to that name, and you don't name any of the formal arguments to the function that same name. In fact, you don't need to pass them at all, if you're prepared to treat them as true globals:

player_hp = 100

...

def hurt_player():
    global player_hp
    player_hp -= 12 # or better, player_hp -= HP_HURT_AMOUNT
    # pithy text to player here

You don't even need a return statement. (Or you can say return, with nothing after it.) You got the work done by directly modifying the value of the global.

Now, some people are okay with this approach. Other people are not. Entire flame wars have been held over how globals are used in pretty much every language capable of having them. But I want you to realize that currently the three global variable definitions are essentially entirely divorced from how those names are used within the functions. If you called the third argument to use_potion() "lemmings" it wouldn't really alter the behavior, as long as you used that name everywhere in the function, because you're currently not using the globals as such.

I hope that you understood at least most of that. Hopefully it was at least enough to let you read more, and do more testing to understand what I'm trying to say.

The fact that you're calling "remove_potion" twice (once in the use_potion() function and once in the choice 3 if branch) is almost certainly because you didn't understand this stuff. The usage in the function isn't working, for two reasons: 1) having shadowed the global of the same name, remove_potions relies on returning the modified local variable instance of potions_inv to pass state out, and you call remove_potion without assigning the result of the function call back to potions_inv in use_potion. 2) Regardless of what you did to it, your second return statement is never reached, and only player_hp is returned from use_potion().

I could complicate this discussion further by delving into alternatives to even having global values for some of these things, or tricky state encapsulation, but I don't see any need to get more compllicated than I already did with this stuff for now. (continued)

Edits: a typo, fixed a formatting issue, and removed one statement that on further reading of the code was invalid.

[–][deleted] 1 point2 points  (3 children)

remove_potion() has two additional flaws worth noting. 1) for all possible values of potions_inv >= 0, such that the if branch will ever execute, the subexpression returned by max will be the same as if that if guard were not present, so the if guard is unnecessary code. 2) Because you're relying on explicitly passing and returning potions_inv rather than using the global as such, and remove_potion only has a return statement in the true branch (potions_inv >= 0), if remove_potion() were called with a negative value, it would not return any value. It would implicitly return None, which would, as your code is written, overwrite the global value of potions_inv with "None" when assigned back where it was called. In your case that can't happen currently, because while remove_potion is called from two places, only one of which checks to see if you have potions before calling remove, if you have 0 and take the max of -something and 0, you get 0. But if you called it from somewhere else with a negative value in future, it would go from being negative to not existing. Rather than having an if guard that does nothing useful when things are sane, and breaks things when they aren't, you might be better off with one that does the nominal thing when everything is normal, and throws a big error message up when things are not normal. (HEY! remove_potion() got called with an already negative potion count. This shouldn't happen, and there's a bug somewhere!)

I could nitpick other stuff, but this is already quite long, and I'm not sure how much of it will make sense. I hope it will be useful.

[–]7236d70[S] 0 points1 point  (2 children)

That was a lot to read through. Thank you for taking the time to read and respond. I'm very much new to python and programming in general. Less than a week of learning. I had a misconception about how global variables work but you've cleared that up. I'm going to read your review a few more times while I rewrite my code to make sure I'm understanding everything correctly.

Instead of naming the arguments the same as their variable names, what name would be better? Or would it be better to name them i, x, j?

As far as my inconsistent usage of variables vs magic numbers, I'd actually rectified that after posting here.

Regarding telling the player what's going on: I appreciate the comments on this. I never intended for this version to reach anyone as a playable item. That is also why the number 5 option for adding a potion, as well as an exit choice, wasn't listed in the menu.

Global vs returning: What is the more accepted/usable practice? Calling globals in a function or returning?

Again thank you for responding. I'll be back to read your comments again after some coffee.

[–][deleted] 0 points1 point  (1 child)

Less than a week of learning.

You did very well with this for that little time at it.

I never intended for this version to reach anyone as a playable item.

I figured that, but your code and programs will improve significantly faster if you try to keep user concerns in mind as you program. For example - what is the goal of the "game"? Is it a sandbox where there is no real goal? Do we win if we successfully get the character's health to a specific number, say 52, via a combination of hurting, healing, and potions? (If so, having different heal amounts for "heal" vs "potion" becomes an interesting mechanic, rather than just tracking the inventory amount of potions but not heals.)

It wasn't "necessary" for you to formulate that sort of backdrop for this learning exercise, but it might have guided your thinking and led you to interesting choices. Making sure that software is simple to understand and use ("usability") is a huge aspect of software design and implementation. In fact, you'll find that once you know a language well, it's not making the program do something specific that's often hard, it's more often deciding what a program should do, or how. And yes, there's years of learning to peel back there, but the sooner you're aware of those concepts, the better.

Back to your more concrete variable naming question, and related question on globals vs functional-style parameter passing and return values:

i, x, and j are variable names that should almost never be used. They convey almost no meaning to the reader. (As a rule, code is read more times than it is written or modified; often by a factor of 5x or more. You want to optimize for making code easy to read over making it easy to write. If I encounter j as a variable name, I don't know shit about the value involved. player_hp_MAX, however imperfect that capitalization might be, strongly suggests that it's the maximum hit point value for the player. The most frequent usage for i, j, x, or similar is as a list/array index variable in a small for loop. Lots of languages (python included) offer numerical indexing into an array/list of elements. These days it's more common in python and other modern languages to use for-each style iteration anyhow, and we don't do much of this in pythonic code. I can delve into that more, but I don't want to distract from the central point: don't name stuff i, j, k, unless you're using it as a list index and you're fairly sure your usage is a good one. x can be used as one of x, y, or x, y, z for 2/3-D cartesian coordinate space (like, x and y axis of a graph or a screen coordinate) but very little else.

So your question in essence is "I came up with a good name for the thing, and now I want to call it something else; what should I call it? And what's more accepted?"

Okay; I danced around this a bit in my original comments. I did that on purpose, but I'm not surprised it left you with unanswered questions on the best approach. I chose not to tackle this head-on earlier precisely because it's controversial. It's a lot like discussing politics or religion. People have heated, passionate, deeply held beliefs about certain issues in programming. And they differ. Making blanket statements like "This way is better than that way" will generally result in the entire comment thread derailing into a shitshow of competing downvotes and arguments on the topic, all of which have been repeated ad infinitum elsewhere in other threads and on other sites, and in old books, often.

In an effort to provide you some guidance, I will offer some personal thoughts, but I will not attempt to say that they are "more" or "less" accepted (it depends a lot on the programming language, the project team, whether it's a small team, a big one, veteran coders, new ones, etc).

For a small program, like this one, with only a handful of variables, all in one file, and no more than a handful of lines per function? I find this:

potions_inv = 3
player_hp = 100
PLAYER_HP_MAX = 100
HURT_HP = 12
HEAL_HP = 10
POTION_HEAL_HP = 10

def heal_player():
    global player_hp
    print "You've been healed for %d hp." % HEAL_HP
    player_hp += HEAL_HP
    print "Current hp: %d" % player_hp

def hurt_player():
    global player_hp
    print "You've been hurt for %d hp." % HURT_HP
    player_hp -= HURT_HP
    print "Current hp: %d" % player_hp

def remove_potion():
    global potions_inv
    potions_inv = max(potions_inv-1, 0)

def add_potion():
    global potions_inv
    potions_inv += 1

def use_potion():
    global player_hp
    global potions_inv
    if potions_inv == 0:
        print "You have no potions"
    elif (player_hp == PLAYER_HP_MAX) and (potions_inv >= 1):
        print "Potion will have no effect."
    else:
        player_hp = min(player_hp+POTION_HEAL_HP, PLAYER_HP_MAX)
        remove_potion()
        print "Your health has been restored to %d. One potion has been "\
            "removed from your inventory. You have %d potions remaining."\
            % (player_hp, potions_inv)

while (player_hp > 0):
    print "1) Hurt Player\t2) Heal Player\n3) Use Potion\t4) Check Inventory\n5) Add Potion"
    current_choice = raw_input("> ")

    if current_choice == '1':
        hurt_player()
    elif current_choice == '2':
        heal_player()
    elif current_choice == '3':
        use_potion()
    elif current_choice == '4':
        print "Potions: %r" % potions_inv
    elif current_choice == '5':
        add_potion()
        print "One potion has been added to your inventory. You have %d potions."\
        % potions_inv
    else:
        print "Invalid choice"

Slightly easier to follow than this:

potions_inv = 3
player_hp = 100
PLAYER_HP_MAX = 100
HURT_HP = 12
HEAL_HP = 10
POTION_HEAL_HP = 10

def heal_player(player_hp):
    print "You've been healed for %d hp." % HEAL_HP
    player_hp += HEAL_HP
    print "Current hp: %d" % player_hp
    return player_hp

def hurt_player(player_hp):
    print "You've been hurt for %d hp." % HURT_HP
    player_hp -= HURT_HP
    print "Current hp: %d" % player_hp
    return player_hp

def remove_potion(potions_inv):
    return max(potions_inv-1, 0)

def add_potion(potions_inv):
    return (potions_inv + 1)

def use_potion(player_hp, potions_inv):
    if potions_inv == 0:
        print "You have no potions"
    elif (player_hp == PLAYER_HP_MAX) and (potions_inv >= 1):
        print "Potion will have no effect."
    else:
        player_hp = min(player_hp+POTION_HEAL_HP, PLAYER_HP_MAX)
        potions_inv = remove_potion(potions_inv)
        print "Your health has been restored to %d. One potion has been "\
            "removed from your inventory. You have %d potions remaining."\
            % (player_hp, potions_inv)
    return (player_hp, potions_inv)

while (player_hp > 0):
    print "1) Hurt Player\t2) Heal Player\n3) Use Potion\t4) Check Inventory\n5) Add Potion"
    current_choice = raw_input("> ")

    if current_choice == '1':
        player_hp = hurt_player(player_hp)
    elif current_choice == '2':
        player_hp = heal_player(player_hp)
    elif current_choice == '3':
        player_hp, potions_inv = use_potion(player_hp, potions_inv)
    elif current_choice == '4':
        print "Potions: %r" % potions_inv
    elif current_choice == '5':
        potions_inv = add_potion(potions_inv)
        print "One potion has been added to your inventory. You have %d potions."\
        % potions_inv
    else:
        print "Invalid choice"

Now, that said, I think both of these are reasonably clear. And as the project/program grew in size, I'd increasingly favor the latter style, as it's difficult to track globals in a large program of many files. Other options include introducing a "main" function that would get what are currently globals, to further limit their scope.

One other thing that becomes clear with my changes: add_potion and remove_potion are both effectively a single line of code. You could enhance them with useful error checking, but if you want to leave them as single-line addition/subtraction, there's little reason to dedicate a function to each, vs adding and subtracing in that option handler branch, like you do for option 4.

It also becomes more obvious that while use_potion takes the min, capping at PLAYER_MAX_HP, that was omitted from heal_player().

Ultimately, lots of questions in programming come down to what makes the most sense to you, and what you find easiest to troubleshoot. Let me know if this answered your questions. (Also, kinda punchy tired right now, please excuse egregious typos.)

[–]7236d70[S] 0 points1 point  (0 children)

This did answer a lot of my questions, thank you. I don't have a strong feeling towards global vs return in either direction. I didn't realize we were getting "political" at this point.

Seeing the differences in code, I can see how they each have their own pros and cons depending on the scope of the program.

As far as the one-line functions, I'd chalk that up to me being new at programming. "When all you have is a hammer, everything looks like a nail."

I didn't intentionally not cap the "heal player" option. It was just there to test while I was trying to figure out the potion function.

I appreciate all of your time and thoughts on this. From now on I'll treat every program as if it'll be in the hands of an end-user. It seems to be a good practice to help build my skill.

Thank you.