I am creating a simple BMI Calculator to check if the user is overweight or not. I am not sure how much improvement I need but I do appreciate any comments as to whether the coding is inconsistent.
def introduction_page():
print("Welcome to the BMI Calculator")
to_start = str(input("Y to start. N to exit"))
if to_start in ("Y","y"):
print("We will calculate now")
return main_page()
else:
print("exiting")
def main_page():
found = False
while not found:
weight_store = float()
height_store = float()
user_weight = float(input("Please enter your weight(kg): "))
weight_confirm = str(input("Y to confirm. N to re-enter"))
if weight_confirm in("y","Y"):
weight_store = weight_store + user_weight
while not found:
user_height = float(input("Please enter your height(m): "))
height_confirm = str(input("Y to confirm. N to re-enter"))
if height_confirm in ("Y","y"):
height_store = height_store + user_height
total_height = (height_store * height_store)
total_weight = (weight_store)
BMI = (total_weight / total_height)
print (int(BMI))
if (BMI < 18.5 or BMI <25) :
print("Normal Weight")
found = True
elif (BMI < 25 or BMI < 30):
print("Over-weight")
found = True
elif (BMI < 30 or BMI < 40):
print("Obesity")
found = True
else:
print("Morbid Obesity")
found = True
else:
print("Please re-enter")
else:
print("Please re-enter!")
introduction_page()
-
\$\begingroup\$ Follow-up question \$\endgroup\$200_success– 200_success2017年07月07日 15:42:16 +00:00Commented Jul 7, 2017 at 15:42
2 Answers 2
Readability
Indentation
Per convention, you should indent your code with 4 spaces.
Refactoring
You should consider refactoring your code. This will allow the code to be more readable and will also allow you to reuse it. As Piotr mentioned it, the four functions can be extracted, which would essentially cover first two of his points.
Otherwise, the variable naming is good and easy to understand, following the PEP-8 guide. You can give the guide a read, to understand and follow a good style practice.
Variable input & logic
If you intend to assign a string to a variable using input
, you don't need to specifically convert it to str
. That is because input
automatically assumes the variable to be a string.
Run the following snippet to see for yourself:
new_variable = input('Enter some text: ')
print(type(new_variable))
Abundance of redundant variables & cluttered logic
Your code has too many variables that hold no real purpose. Your store
and total
variables are redundant, as you can work directly off your input.
Operators
height_store = height_store + user_height
total_height = (height_store * height_store)
You can re-write the above two lines using operators. They would look like this:
height_store += user_height
total_height = height_store ** 2
We won't use the first operator in this instance, as it won't be needed. We can narrow that block of logic down to the following code, which still works as intended.
user_weight = float(input('Please enter your weight(kg): '))
user_height = float(input('Please enter your height(m): '))
body_mass_index = (user_weight / user_height ** 2)
print(round(body_mass_index, 1))
BMI Result
The direction of comparison operators doesn't seem to be correct in your block of if statements. If statements don't require parentheses.
if 18.5 < body_mass_index < 25:
print('Normal weight')
elif 25 < body_mass_index < 30:
print('Overweight')
elif body_mass_index > 30:
print('Obese')
else:
print('Underweight')
Also, you seemed to be missing a check for an underweight BMI.
Refactoring
Now that the logic is no longer cluttered, we can put it into different functions. Within those, we can restrain user input to certain values and handle errors. We will also some of the logic into a class.
As the tallest man ever was 2.72 meters tall, we are going to use that as the upper bound for _get_user_info
.
class BodyMassIndex:
def __init__(self, weight, height):
self.weight = weight
self.height = height
@property
def body_mass_index(self):
return round(self.weight / self.height ** 2, 1)
@property
def score(self):
if 18.5 < self.body_mass_index < 25:
return 'normal weight'
elif 25 < self.body_mass_index < 30:
return 'overweight'
elif self.body_mass_index > 30:
return 'obese'
else:
return 'underweight'
def print_score(self):
print('Your Body Mass Index score is: {}'.format(self.body_mass_index))
print('You are {}'.format(self.score))
def _get_user_info():
while True:
try:
weight = float(input('Enter weight in kilograms: '))
height = float(input('Enter height in meters: '))
if 0 < weight and 0 < height < 2.72:
return weight, height
else:
raise ValueError('Invalid height or weight')
except ValueError:
print('Invalid height or weight input')
continue
def calculate_bmi():
weight, height = _get_user_info()
return BodyMassIndex(weight, height)
if __name__ == '__main__':
bmi = calculate_bmi()
bmi.print_score()
calculate_bmi
functions gets user input and returns an instance of BodyMassIndex
class, which performs the logic. If you want to handle weight and height input separately, you can split _get_user_info
into two separate functions, _get_user_weight
and _get_user_height
.
Now, when we run the code, this is how it will behave:
Enter weight in kilograms: 78
Enter height in meters: 1.8
Your Body Mass Index score is: 24.1
You are normal weight
The code is perfectly reusable and you can use it as a module and import it in other programs.
>>> bmi = calculate_bmi()
Enter weight in kilograms: 78
Enter height in meters: 1.8
>>> bmi.height
1.8
>>> bmi.weight
78.0
>>> bmi.body_mass_index
24.1
>>> bmi.score
'normal weight'
for a starter I see tree aspects which you could upgrade:
- Improve code readability
- Make code reusable
- Improve validation / error handling.
From main_page()
extract 4 functions:
get_user_height()
get_user_weight()
calculate_bmi(height, weight)
print_bmi_description(bmi)
In get_user_height()
and get_user_weight()
user input. What is min and max value which are correct in your model? e.g. nobody has weight of -1.0.
Make sure that you can trust this functions i.e. they return only valid values.
calculate_bmi(height, weight)
- extracting calculation code to distinct function makes it easy to reuse and test (unit tests or even simpler in interactive console).
print_bmi_description(bmi)
- make sure your logic covers all possible values. By covers I mean it returns correct description or throws an error (ie. for negative BMIs).
- You should remove redundant code eg.
BMI < 18.5 or BMI <25
- check only forBMI <25
. - Validate your logic.
BMI < 30 or BMI < 40
seems incorrect. I think it should beBMI > 30 or BMI < 40
or in more pythonic way30 < BMI < 40
I think it is enough for know. If you make this changes we could make next step :-)
-
\$\begingroup\$ Thank you for the comments! out of 5, please rate me. 1 (bad), 5(good) \$\endgroup\$Anson Kho– Anson Kho2017年07月03日 01:52:29 +00:00Commented Jul 3, 2017 at 1:52
-
2\$\begingroup\$ It's great you are looking for feedback. Forget about ratings, just make sure you deliver your best and make it better every time. According to Wikipedia article regarding BMI, your logic (ifs) is incorrect. Correctness is the most important property of good code - so make it work properly first. So for this time I would give you 1. Good luck with your journey! \$\endgroup\$Piotr Nawrot– Piotr Nawrot2017年07月06日 19:34:48 +00:00Commented Jul 6, 2017 at 19:34
-
\$\begingroup\$ Thank you for your kind words! will definitely work hard!:) \$\endgroup\$Anson Kho– Anson Kho2017年07月07日 13:28:48 +00:00Commented Jul 7, 2017 at 13:28