5
\$\begingroup\$

I made a code for a health system for the game I'm creating. I made it myself and I wanted some criticism on how to make it better. Thanks for viewing.

## Health system
import time
SH = 100
PA = True
while PA == (True):
  DMG = input("How much damage would you like to take?" )
  Output = int(SH) - int(DMG) 
  if Output == (80):
    print("You took 20 damage!")
    print(" --------------")
    print("|■■|■■|■■|■■|  |")
    print(" --------------")
  elif Output == (60):
    print("You took 40 damage!")
    print(" --------------")
    print("|■■|■■|■■|  |  |")
    print(" --------------")
  elif Output == (40):
    print("You took 60 damage!")
    print(" --------------")
    print("|■■|■■|  |  |  |")
    print(" --------------")  
  elif Output == (20):
    print("You took 80 damage!")
    print(" --------------")
    print("|■■|  |  |  |  |")
    print(" --------------")
  elif Output == (0):
    print("You took 100 damage!")
    print(" --------------")
    print("|  |  |  |  |  |")
    print(" --------------")
    print("You died!")
  else:
    print("That isn't a command")

  PA = input("Would you like to play again? |Y/N| ")
  if PA == ("Y"):
    PA = True
  elif PA == ("N"):
    PA = False
    print("The program will now close")
    time.sleep(5)
  else:
    print("That is not a command...")
\$\endgroup\$
  • \$\begingroup\$ Why have that time delay before closing? \$\endgroup\$ – Malady May 10 '18 at 18:40
12
\$\begingroup\$

I gotta start by saying it's weird to ask how much damage you want to take, my answer would always be zero, you know!

Your variable names could be more descriptive.

I understand that DMG means damage, SH would be.. something health?

Well I guess you see my point, if we don't know what the variable name means, it's harder to figure out what it does.

Considering the PEP8 guidelines towards Python, your variable should never contain upper case.

So I would change :

DMG -> damage
SH -> total_health
Output -> remaining_health
PA -> play_again

In your input you ask how much damage should be dealt. There are much more options that are invalid than valid ones. I mean, I can enter 0,20,40,60,80,100 but I can't enter any other number, so there are much more chances I face the "That isn't a command" error message than anything else.

If the only options are the values from 0 to a 100 that are multiples of twenty, you should specify it in your message :

"How much damage would you like to take? [0,20,40,60,80,100]"

There is no real advantage for the Output variable at this moment. You want to print how much damage is dealt and an health bar that corresponds to the remaining life.

if damage == 20: print("You took 20 damage!")

Or, even better, use the .format method.

print("You took {0} damage!".format(str(damage))

Then you could print the health bar. I don't want to go deep in details but you could write a function like print_remaining_health_bar(total_health,remaining_health) that would print according to the parameters instead of repeating this piece of code every time.

All in all you could remove the whole if/else business (except for your input validation, which is important) and have :

print("You took {0} damage!".format(str(damage))
remaining_health = total_health - damage
print_remaining_health_bar(total_health,remaining_health)

There's also a small problem with your code. When asked if I want to play again, if I enter an invalid command, I'd just restart the game.

\$\endgroup\$
  • 1
    \$\begingroup\$ SH actually does appear to be a constant as written so it might be more appropriate to use TOTAL_HEALTH according to PEP8 \$\endgroup\$ – poompt May 10 '18 at 22:30
  • \$\begingroup\$ Just in terms of user experience, most CLI programs asking for input use parentheses for a list of allowed responses, e.g. Do you wish to continue (y/n)? [y] ; Enter a number (1-99): ; Enter your name: [leave empty] , where brackets denote the default option. \$\endgroup\$ – Daniel May 11 '18 at 14:17
4
\$\begingroup\$

I'll echo @TopinFrassi to have better-named variables.

Since you don't mention a limit on the amount of damage you can take. I believe you need to change your prompt to be something like (and then you'll want to check that they did):

damage = input("How much damage would you like to take?(interval of 20)" )

Or change your logic after your current prompt. First, you should try casting the damage right away and surround it with a try...except in case they enter a string. Then have a single line to print how much damage was done, even if you stick to multiples of 20. I would make a print_health function that handles the health bar so that you can get even more precise. If your health is less than or equal to 0 you should stop the game.

While asking to play again it should probably be in a loop in case they enter something invalid. And then I'd move the sleep and print outside of the loop.

import time
health= 100
play = True

def print_health(hp):
    l='■'
    s='|'
    for i in range(10):
        if i<hp/10:
          s+=l # show a bar of health if multiple of 10
        else:
          s+=" " # else fill the missing hp with space
        if i%2==1:
          s+='|' # print a | every 2 characters
    print(s)

while play:
  try:
      damage= int(input("How much damage would you like to take?" ))
  except ValueError:
      print("That isn't a command")
      continue # starts the loop over
  remaining_health = health - damage
  print("You took {0} damage!".format(damage))
  health=remaining_health
  print(" --------------")
  print_health(remaining_health)
  print(" --------------")

  if remaining_health <= 0:
    print("You died!")
    break # exit the loop

  again = 'x'
  while again not in 'YN': # you may want to cast againt to upper
      again = input("Would you like to play again? |Y/N| ")
      if again == "N":
        play = False
      elif again != 'Y':
        print("That is not a command...")
print("The program will now close")
time.sleep(5)
\$\endgroup\$
  • \$\begingroup\$ except:? Seriously? \$\endgroup\$ – d33tah May 10 '18 at 21:04
  • \$\begingroup\$ @Graipher that is correct, missed that, have since fixed. However, I do not see the issue with except (is it because I'm not specifying the error type?) \$\endgroup\$ – depperm May 10 '18 at 22:26
  • 2
    \$\begingroup\$ You should know what exceptions might be raised. Don't catch all of them. \$\endgroup\$ – hjpotter92 May 10 '18 at 22:49
  • 1
    \$\begingroup\$ Also you are setting PA = False instead of play. \$\endgroup\$ – hjpotter92 May 10 '18 at 22:52
  • \$\begingroup\$ @depperm Yes, it is. A bare except even catches the user trying to press Ctrl+C. Use except ValueError instead. \$\endgroup\$ – Graipher May 11 '18 at 6:13
1
\$\begingroup\$

The main problem with your code is that there is no way to reuse it or reuse any part of it (apart from PA and SH by importing the module you designed, but they obviously can not be of great use).

I suggest you to redesign your code in terms of functions (this is the minimum, otherwise I prefer the object oriented approach even if some may argue against it).

You declared PA = True. That means it is a constant, but later in your code you write PA = input("Would you like to play again? |Y/N| ") ... that does not make sense.

\$\endgroup\$

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.