0
\$\begingroup\$

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()
\$\endgroup\$
  • 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 Jul 7 '17 at 15:44
  • \$\begingroup\$ Stop writing classes \$\endgroup\$ – jonrsharpe Jul 8 '17 at 12:39
6
\$\begingroup\$

Introduction

After reflecting and reading other answers, I found that Adam Smith is right: we should git 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 or False) that asks whether the user wants to start. We will rename to_start to answer. Using upper 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 of if __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.

\$\endgroup\$
  • 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 the wants_to_start function a lot neater. ;) \$\endgroup\$ – Lukasz Salitra Jul 7 '17 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\$ – Mahmud Muhammad Naguib Jul 8 '17 at 11:52
2
\$\begingroup\$

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)
    ...
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I believe it would be handy to include self.weight and self.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\$ – Lukasz Salitra Jul 8 '17 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 Jul 8 '17 at 5:23

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.