Living off of chocolate

Question

This is the second project for my CS1 class, this time I'm actually getting it reviewed before I submit it ;)

The Harris-Benedict equation estimates the number of calories your body needs to maintain your weight if you do no exercise. This is called your basal metabolic rate, or BMR.

The formula for the calories needed for a woman to maintain her weight is

$$ \text{BMR} = 655 + \left( 4.3 \cdot \text{weight in pounds} \right) + \left( 4.7 \cdot \text{height in inches} \right) - \left( 4.7 \cdot \text{age in years}\right)$$

The formula for the calories needed for a man to maintain his weight is

$$ \text{BMR} = 66 + \left( 6.3 \cdot \text{weight in pounds} \right) + \left( 12.9 \cdot \text{height in inches} \right) - \left( 6.8 \cdot \text{age in years}\right)$$

A typical chocolate bar will contain around 230 calories. Write a program that allows the user to input his or her weight in pounds, height in inches, age in years, and the character 'M' for male and 'F' for female. The program should then output the number of chocolate bars that should be consumed to maintain one's weight for the appropriate sex of the specified weight, height, and age.

calorie.cpp:

/**
 * @file calorie.cpp
 * @brief Determines the number of chocolate bars you need to eat in order to live
 * @author syb0rg
 * @date 9/12/14
 */

#include <iostream>
#include <cctype>
#include <limits>

int main()
{
    double weight = 0;
    double height = 0;
    double age = 0;
    double bmr = 0;
    char gender = 'm';

    // get input for weight, re-read input if not a number
    std::cout << "Enter the weight in pounds: ";
    while(!(std::cin >> weight))
    {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        std::cout << "Invalid input.  Please enter a number: ";
    }
    // get input for height, re-read input if not a number
    std::cout << "Enter the height in inches: ";
    while(!(std::cin >> height))
    {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        std::cout << "Invalid input.  Please enter a number: ";
    }
    // get input for age, re-read input if not a number
    std::cout << "Enter your age: ";
    while(!(std::cin >> age))
    {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        std::cout << "Invalid input.  Please enter a number: ";
    }
    // get input for weight
    std::cout << "Enter your gender (M/f): ";
    std::cin >> gender;

    if ('m' == gender)
    {
        bmr = 66 + (6.3 * weight) + (12.9 * height) - (6.8 * age);
    }
    else if ('f' == gender)
    {
        bmr = 655 + (4.3 * weight) + (4.7 * height) - (4.7 * age);
    }
    else std::cout << "You aren't human." << std::endl;

    const double numBars = bmr / 230; // divide by the number of calories in a chocolate bar
    std::cout << "The person with the entered metrics is allowed to consume " << numBars << " bars of chocolate." << std::endl;
}

Answer 1

Just a quick glance. But:

std::cout << XXXX
while(!(std::cin >> age))
{
    std::cin.clear();
    std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    std::cout << "Invalid input.  Please enter a number: ";
}

That could be a function.
This would reduce code and make the application easier to read.

Remember code should be DRY.

The formula is the same here:

if ('m' == gender)
{
    bmr = 66 + (6.3 * weight) + (12.9 * height) - (6.8 * age);
}
else if ('f' == gender)
{
    bmr = 655 + (4.3 * weight) + (4.7 * height) - (4.7 * age);
}

So why not pick the constants from some indexed value:

bmr = bmrconst[gender].base 
      + (bmrconst[gender].weight * weight) 
      + (bmrconst[gender].height * height) 
      - (bmrconst[gender].age    * age);

but pick better names than I did.

Don't like magic numbers (230).

const double numBars = bmr / 230;

Use a constant that has a name that makes this have meaning.

Don't declare all your variables at the top:

double weight = 0;
double height = 0;
double age = 0;
double bmr = 0;
char gender = 'm';

Declare them as you need them as close to the point of first use as possible.

Answer 2

The gender selection process isn't user-friendly.

In the prompt, you capitalized M but not f. I take that to mean that M is the default, but that turns out not to be the case.

Be lenient in what you accept. Treating either uppercase or lowercase input as the same would lead to a better user experience.

Not all people fit within the binary gender system. While you don't have to embrace diversity to the extent that Facebook does, it is absolutely inappropriate to say anything that might be construed as implying that transgender people are subhuman.

Answer 3

In addition to moving these declarations closer to where you use them as @lokiastari said, the initializations are pointless because you overwrite them anyway. So change that to simply:

double weight;
double height;
double age;
double bmr;
char gender;

It's recommended to use braces around if and else statements:

else {
    std::cout << "You aren't human." << std::endl;
}

It's a bit inconsistent that after you've established the user not actually a human, you go ahead anyway and print the message "The person with the entered metrics ...".

An if-else chain like this if ('x' == ...) else if ('y' == ...) calls for a switch:

switch (gender) {
    case 'm':
        bmr = ...
        break;
    case 'f':
        bmr = ...
        break;
    default:
        ...
}

Of course, the suggestion by @lokiastari to use constants from some indexed value for this is even better.

I know you commented that your CS class hasn't covered functions yet, but you do know C, and at the minimum you could have put this much in a super simple parameterless void function:

std::cin.clear();
std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
std::cout << "Invalid input.  Please enter a number: ";

There's no excuse for copy-pasting when you do know better, and it makes the code review easier too, as we don't have to re-read those lines multiple times to verify if they are really identical.

Some extra tips, only very loosely related to this review, but for your C++ learning experience:

• When you start wondering about the difference between std::endl and \n, this post is very helpful
• Sometimes you might want to print on stderr instead of stdout (possibly as a response to invalid input in your case), you can do that with std::cerr << "..."

Answer 4

Although not required here, you could consider putting the variables into a struct:

struct Profile
{
    double weight;
    double height;
    double age;
    double bmr;
    char gender;
};

If I'm not mistaken, C structs do not have constructors, meaning you would've had to use a separate function to handle this instead.

In C++, this is not the case!

struct Profile
{
    double weight;
    double height;
    double age;
    double bmr;
    char gender;

    // this is an initializer list
    // the format is "field(value)"
    Profile(double weight, double height,
        double age, double bmr, char gender)
        : weight(weight)
        , height(height)
        , age(age)
        , bmr(bmr)
        , gender(gender)
    {}
};

Essentially, you can set the fields to the desired values right away, instead of giving them default values just for placeholders. If at any point you do want default values, then you simply replace the argument in the list with the desired value (and you may also remove that argument entirely).