4
\$\begingroup\$

Well, that's my first "program" after couple days of working with Python and programming itself. That may be stupid since everything there is so basic but I wanted to ask if there are any spaces to improve it. Idea was to give the user proper dosage of 4 supplements regarding if this is training day or not.

Is there any way to achieve it faster, more simple, better? Appreciate all feedback!

Edit

Thanks for all your replies! I will go through all of them and learn from them what i can, still confused in programming world.

dayon = {"BCAA" : "10g",
  "White" : "50g",
  "Glutamine" : "20g",
  "Vitamins" : 10}

dayof= {"BCAA" : "5g",
    "White" : "25g",
    "Glutamine" : "10g",
    "Vitamins" : 5}

username = input("What is your name?\n")
trening = input("Are you going to the gym today?\n")

def dosage():
    if trening == "yes":
       return (username + ", you have to take: " +
        "\nBCAA in dosage of: " + str(dayon["BCAA"]) +
        "\nWhite in dosage of: " +str(dayon["White"]) +
        "\nGlutamine in dosage of: " + str(dayon["Glutamine"]) +
        "\nVitamins in number of: " +str(dayon["Vitamins"]))
    elif trening == "no":
        return (username + ", you have to take: " +
        "\nBCAA in dosage of: " + str(dayof["BCAA"]) +
        "\nWhite in dosage of: " + str(dayof["White"]) +
        "\nGlutamine in dosage of: " + str(dayof["Glutamine"]) +
        "\nVitamins in number of: " + str(dayof["Vitamins"]))
    else:
        print ("Please answer yes or no")


print (dosage())
\$\endgroup\$
7
\$\begingroup\$

Here is the list of things I would improve:

  • for handling the proper answer on the "Are you going to the gym today?", I would start the while loop until "Yes" or "No" is entered
  • put the execution logic under if __name__ == '__main__':
  • define a message template with appropriate placeholders - format it with "day on" or "day off" objects and the entered username
  • define constants in upper case as per PEP8
  • let the "dosage" function return the dosage object - print the message outside of the function - for the better logic separation
  • add meaningful comments and docstrings

The code with all the suggested changes applied:

DAY_ON = {
    "BCAA": "10g",
    "White": "50g",
    "Glutamine": "20g",
    "Vitamins": 10
}

DAY_OFF = {
    "BCAA": "5g",
    "White": "25g",
    "Glutamine": "10g",
    "Vitamins": 5
}

MESSAGE_TEMPLATE = """
{username}, you have to take:
    BCAA in dosage of: {obj[BCAA]}
    White in dosage of: {obj[White]}
    Glutamine in dosage of: {obj[Glutamine]}
    Vitamins in number of: {obj[Vitamins]}
"""


def get_dosage(is_training_day):
    """Returns appropriate dosage depending on whether it is a training day or not"""
    return DAY_ON if is_training_day else DAY_OFF


if __name__ == '__main__':
    username = input("What is your name?\n")

    while True:
        choice = input("Are you going to the gym today?\n")

        if choice in ('Yes', 'No'):
            break
        else:
            print("You can only enter 'Yes' or 'No'.")

    dosage = get_dosage(is_training_day=choice == 'Yes')
    print(MESSAGE_TEMPLATE.format(username=username, obj=dosage))

Demo:

$ python3 test.py 
What is your name?
Chubaka
Are you going to the gym today?
What?
You can only enter 'Yes' or 'No'.
Are you going to the gym today?
No

Chubaka, you have to take:
    BCAA in dosage of: 5g
    White in dosage of: 25g
    Glutamine in dosage of: 10g
    Vitamins in number of: 5

If you are using Python3.6+, you can also use f-strings for string formatting:

dosage = get_dosage(is_training_day=choice == 'Yes')
print(f"""
{username}, you have to take:
    BCAA in dosage of: {dosage['BCAA']}
    White in dosage of: {dosage['White']}
    Glutamine in dosage of: {dosage['Glutamine']}
    Vitamins in number of: {dosage['Vitamins']}
""")
\$\endgroup\$
  • 4
    \$\begingroup\$ I'd personally recommend .lower()ing the input(). Apart from this, nice answer \$\endgroup\$ – Grajdeanu Alex. Feb 9 '17 at 20:16
5
\$\begingroup\$

1.- Follow convention when using names (ie, you should use_underscore). If are constant then USE_UPPERCASE. (From dayon to DAY_ON)

2.- If you have numeric variables that are being used as a strings (I mean, you are not expecting to calculate something with them) then declare them as strings. (From "Vitamins": 10 to 'Vitamins': '10')

3.- If you have an dictionary (DAY_ON/DAY_OFF) try to put elements of the same type. Is not a good idea mix strings with integers (or with another type) (Another reason for: From "Vitamins": 10 to 'Vitamins': '10')

4.- A good practice is declaring create methods that only does one thing. For example one method (print_doses) just print the doses (doesn't care about user input or type of the dose), another (calculate_dosage) just calculate the dose according an parameter (does't care about the user input)

5.- Then, trying to add robustness, what if the user doesn't write any name? What if the user write 'YES' or 'Yes' instead of 'yes'? The last lines try to avoid those problems. Also you can create a method for this.

6.- Edit, adding Graipher feedback; Fixing string comparing using '==' instead of 'is'


DAY_ON = {'BCAA': '10g',
          'White': '50g',
          'Glutamine': '20g',
          'Vitamins': '10'}

DAY_OFF = {'BCAA': '5g',
           'White': '25g',
           'Glutamine': '10g',
           'Vitamins': '5'}

POSITIVE_ANSWERS = ['yes', 'y', 'true', '1']
NEGATIVE_ANSWERS = ['no', 'n', 'false', '0']
VALID_ANSWERS = POSITIVE_ANSWERS + NEGATIVE_ANSWERS

INVALID_ANSWER = "Insert a valid name and a valid answer [yes] or [no]"


def print_doses(name, daily_doses):
    print('{0}, you have to take:'.format(name))

    for key in daily_doses.keys():
        print('{0} in dosage of: {1}'.format(key, daily_doses[key]))


def calculate_dosage(name, is_training):
    doses = DAY_ON if is_training else DAY_OFF
    print_doses(name, doses)


username = input('What is your name?\n')
training = input('Are you going to the gym today?\n')

if username is None or training not in VALID_ANSWERS:
    print(INVALID_ANSWER_MSGE)
else:
    is_training = str.lower(training) in POSITIVE_ANSWERS
    calculate_dosage(username, is_training)
\$\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.