I wrote a BMI Calculator. I tested the code and everything seems to be working fine. I would like to check if there is any room for improvement.
class bmi:
def __init__(self):
to_start = input("Y to start N to exit: ")
if to_start in ("Y","y"):
found = False
while not found:
weight_input = float(input("Please enter your weight(kg): "))
if weight_input < 0:
print("Invalid Value")
else:
weight_confirm = input("Y to confirm. N to exit: ")
if weight_confirm in ("Y","y"):
while not found:
height_input = float(input("Please enter your height(m): "))
if height_input < 0:
print("Invalid Value")
else:
BMI = round(weight_input/(height_input*height_input),2)
print ("Your BMI is: ",BMI)
if BMI < 18.5:
print("Under-Weight")
found = True
elif 18.5 <= BMI <= 24.99:
print("Healthy Weight")
found = True
elif 25.0 <= BMI <= 29.99:
print("Over-weight")
found = True
elif BMI > 30:
print("Obese")
found = True
else:
print("Re-enter")
found = False
else:
exit()
if __name__ == '__main__':
a = bmi()
-
3\$\begingroup\$ As a courtesy to users who take the time to review your code, please declare that this is a second version of a previous question, and note what you changed. \$\endgroup\$200_success– 200_success2017年07月07日 15:44:32 +00:00Commented Jul 7, 2017 at 15:44
-
\$\begingroup\$ Stop writing classes \$\endgroup\$jonrsharpe– jonrsharpe2017年07月08日 12:39:44 +00:00Commented Jul 8, 2017 at 12:39
3 Answers 3
Introduction
After reflecting and reading other answers, I found that Adam Smith is right: we should get rid of the class. For this problem I think that it's better to structure the program as a group of functions. This is my new answer.
Style
Variable names should be lowercase, so your variable BMI
should be renamed bmi
.
Modularity
According to Wikipedia, "In software design, modularity refers to a logical partitioning of the "software design" that allows complex software to be manageable for the purpose of implementation and maintenance. The logic of partitioning may be based on related functions, implementation considerations, data links, or other criteria." So you should divide your code into smaller units, for this purpose we will divide your code into a group of functions.
This is better, because it's a bad practice, IMHO, to write all your code as a single whole without dividing it into units. This makes your code reusable, readable, and maintainable.
Documentation
It's a good practice, IMHO, to document your code, this makes it more readable and easier to use. For this purpose we will use docstrings to document the functions that we'll define. Docstings are indented and contained into triple quotes, they describe what the function does, its arguments, the type of its arguments, its return value and the type of the return value (when used with functions). Docstrings are the right way to document functions, not comments, because if you need help about using the function, you can use help(name here)
to display the docstring and get help, which you can't do with comments.
Dividing the Code into Functions
To achieve modularity, we'll divide the code into functions. We'll define:
a boolen-valued function (a function that returns only
True
orFalse
) that asks whether the user wants to start. We will renameto_start
toanswer
. Usingupper
method, we'll make the input uppercase, so we can compare it to 'Y' and 'N' easily if the user entered 'y' or 'n' instead.a function that handles both weight and height.
a function that takes calculates the BMI and returns it.
a function that returns the weight status.
a function called
main
that contains all the code that will be executed. We'll use it as the body ofif __name__ == '__main__'
by calling it.
Return Value
Instead of making our new functions print the output, we'll make them return it. You can read about return values here (this is a link to chapter in Think Python 2e, I recommend that you read the whole book). Just printing the output gets the job done but makes functions less reusable. Suppose that you want to store the BMIs associated with users in a textfile, what will you do? Sure you can make other functions to get this done, but if you make your functions return the result you can reuse many cases.
Code after Refactoring
def body_mass_index(weight, height):
"""Returns the BMI based on weight and height.
weight: float or int
height: float or int
Returns: float or int
"""
return round(weight / height ** 2 , 2)
def weight_category(bmi):
"""Returns the weight category of the user based on their BMI.
Returns: str
"""
if bmi < 18.5:
return 'underweight'
elif 18.5 <= bmi <= 24.99:
return 'healthy weight'
elif 25.0 <= bmi <= 29.99:
return 'overweight'
elif bmi > 30:
return 'obese'
def wants_to_start():
"""Asks whether the user wants to start.
Returns: bool
"""
while True:
answer = input("Y to start N to exit: ").upper()
if answer == 'Y':
return True
elif answer == 'N':
return False
print('Please try again.\n')
def get_user_data():
"""Asks for the user's weight and height.
Rerurns: tuple of 2 floats or integers
"""
while True:
try:
weight = float(input('Enter your weight in kilograms: '))
height = float(input('Enter your height in meters: '))
if 0 < weight and 0 < height:
return weight, height
else:
raise ValueError()
except ValueError:
print('Invalid input for weight or height')
continue
def main():
if wants_to_start():
weight, height = get_user_data()
bmi = body_mass_index(weight, height)
category = weight_category(bmi)
print('Your BMI is: ', bmi)
print('Your are ', category, '.')
else:
quit()
if __name__ == '__main__':
main()
Notes
The function get_user_data
is adopted from Lukasz Salitra's function _get_user_info
in his answer to the OP's previous question. You can find the question here.
I'm no expert on BMI, so I've no idea if the program does what it's supposed to do properly. I focused on making the code more reusable, readable, and well-structured, according to what I know as a beginner.
-
1\$\begingroup\$ Nicely written and as a fellow beginner, thanks for considering my answer in yours. Look up the use of
strtobool
and you will be able to make thewants_to_start
function a lot neater. ;) \$\endgroup\$Luke– Luke2017年07月07日 23:38:12 +00:00Commented Jul 7, 2017 at 23:38 -
\$\begingroup\$ @LukaszSalitra Thank you for your kind words. I learned from your answer and I like the way you wrote
_get_user_info
. Thanks for your suggestion. \$\endgroup\$Mahmood Muhammad Nageeb– Mahmood Muhammad Nageeb2017年07月08日 11:52:45 +00:00Commented Jul 8, 2017 at 11:52
Is there a good reason we're wrapping this in a class? Unless you're creating a whole bunch of these objects (which might then be better named Person
with a property bmi
and bmi_desc
. Logically, why would we have more than one BMI? What would that even mean?)
This seems very functional to me, not object-oriented. Let's drop the class completely.
def run():
# Get weight
while True:
try:
weight = float(input("Please enter your weight(kg): "))
except ValueError:
continue # ask again
else:
if weight < 0:
print("Invalid value")
continue # ask again
else:
break # keep going
# Get height
while True:
try:
height = float(input("Please enter your height(m): "))
except ValueError:
continue # ask again
else:
if height < 0:
print("Invalid value")
continue # ask again
else:
break # keep going
bmi = round(weight / (height**2), 2)
# N.B. I changed the ranges here slightly. You had a subtle bug previously -- see if you can find it!
if bmi < 18.5:
print("Under-Weight")
elif 18.5 <= bmi < 25:
print("Healthy Weight")
elif 25 <= bmi < 30:
print("Over-weight")
elif bmi >= 30:
print("Obese")
run()
You'll notice you're getting two bounded values. You could refactor those.
def bounded_input(prompt, limitlow=None, limithigh=None, typecast=None):
while True:
user_in = input(prompt)
if typecast is not None:
try:
user_in = typecast(user_in)
except Exception:
print("Input value {} cannot be coerced to required type {}".format(
user_in, typecast))
continue # skip the rest of validation
if limitlow is not None:
if user_in < limitlow:
print("Input value {} is beneath minimum value ({})".format(
user_in, limitlow))
elif limithigh is not None:
if user_in > limithigh:
print("Input value {} is above maximum value ({})".format(
user_in, limithigh))
else:
return user_in
Then rewrite the "get height" and "get weight" portions as:
def run():
weight = bounded_input("Please enter your weight(kg): ",
limitlow=0,
typecast=float)
height = bounded_input("Please enter your height(m): ",
limitlow=0,
typecast=float)
...
This is my answer building on the answer of @Mahmud Muhammad Naguib I made some slight adjustments for an overall ok program.
Changes
Weight and height were never used as unique values but rather as input for the BMI. In my program the bmi and corresponding category is automatically generated.
Secondly I made the case class a __str__
representation, that way after Init you can just do print(new_bmi)
class BodyMassIndex:
"""Represents BMI.
attributes:
weight: float
height: float
"""
def __init__(self, weight, height):
"""Initializes a BodyMassIndex object."""
self.bmi = round(weight / height **2, 2)
if self.bmi < 18.5:
self.catagory = 'underweight'
elif 18.5 <= self.bmi < 25:
self.catagory = 'healthy weight'
elif 25 <= self.bmi <= 30:
self.catagory = 'overweight'
else:
self.catagory = 'obese'
def __str__(self):
"""Returns a print of the bmi."""
return 'Your bmi is {0} and you are {1}'.format(self.bmi, self.catagory)
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:
return weight, height
else:
raise ValueError('Invalid height or weight')
except ValueError:
print('Invalid height or weight input')
continue
def main():
while True:
answer = input("Start/Restart [Y/N]>>> ")
if answer in ['Y','y']:
weight, height = get_user_info()
new_bmi = BodyMassIndex(weight, height)
print(new_bmi)
else:
quit()
if __name__ == '__main__':
main()
Note
As I just found out this is a follow up question I think the original answer of BMI Calculator using Python 3 was the best.
-
\$\begingroup\$ I believe it would be handy to include
self.weight
andself.height
in the__init__
method, so the values can be accessed if needs be. And thanks for note at the end of your answer, appreciate it. :) \$\endgroup\$Luke– Luke2017年07月08日 00:05:05 +00:00Commented Jul 8, 2017 at 0:05 -
\$\begingroup\$ @LukaszSalitra You are right, Properties would be the way to go, but hen I would have almost an exact replica of yours. So I couldn't be bothered anymore, only improvement I could make was the
__str__
\$\endgroup\$Ludisposed– Ludisposed2017年07月08日 05:23:28 +00:00Commented Jul 8, 2017 at 5:23