BMI Calculator

[gcorsini94]

Hello. One of my recent projects was a BMI calculator. Nothing but entering height and weight. I used a function for the Height/Weight calculation because functions are the most recent topic I learned. I wanted to see if the Format for my code was clean and well written. I don't like having bad habits and would like to rid myself of them early on.

#include <iostream>
#include <string> 
using namespace std;

void bmi();

int main() {

	bmi();

	cin.get();
	return 0;
}


void bmi () {


	double h; 

	cout << "Height in inches: "
		 << endl;
	cin >> h;
	cout << "You are " << h << " inches. Fantastic. Moving on."
		 << endl;

	double w;

	cout << "Please enter your weight in lbs: "
		 << endl;
	cin >> w;
	cout << "You weigh " << w << " lbs. Let's see." 
		 << endl;

	double x;

	x = ( w / (h * h)) * 703;

	cout << "Your BMI is " << x << "."
		 << endl;

	if (x <= 18.5)
		cout << "You're underweight!"
			 << endl;

	else if (x >= 18.5, x <= 24.9)
		cout << "You're normal weight!"
			 << endl;

	else if ( x >=25.0, x <= 29.9)
		cout << "You're overweight!"
			 << endl;

	else if ( x >= 30)
		cout << "You're obese!"
			 << endl;

	cin.get();
	
}

I appreciate any feedback.

[andywestken]

I appreciate any feedback.

#1 you have the same bug on lines 46 and 50

To combine conditions you need to use the logical operators || or &&, not the comma operator.

e.g. x >= 18.5 && x <= 24.9 rather than x >= 18.5, x <= 24.9

The comma operator is also know as the sequence operator, as it causes things to be done in sequence from left to right. The return value is the result of the first thing done (here x >= 18.5)

For example (note that the last pair of values disagree.)

#include <iostream>
#include <iomanip>
using namespace std;

void test(int x) {
	bool seq_op = false;
	bool and_op = false;
	seq_op = x >= 18.5, x <= 24.9;
	and_op = x >= 18.5 && x <= 24.9;
	cout << "for x = " << x << endl;
	cout << "x >= 18.5, x <= 24.9   -> " << seq_op << endl;
	cout << "x >= 18.5 && x <= 24.9 -> " << and_op << endl;
	cout << endl;
}

int main() {
	cout << boolalpha; // display true and false as words

	test(17.5);
	test(22.0);
	test(75.0);

	return 0;
}

Results:

for x = 17
x >= 18.5, x <= 24.9   -> false
x >= 18.5 && x <= 24.9 -> false

for x = 22
x >= 18.5, x <= 24.9   -> true
x >= 18.5 && x <= 24.9 -> true

for x = 75
x >= 18.5, x <= 24.9   -> true
x >= 18.5 && x <= 24.9 -> false

Also be aware of the interaction between operator, and operator()

(where a, b, c are all ints)

a = b, c;    // a is set to the value of b
a = (b, c);  // a is set to the value of c 

#2 General comments

It is good that you declare your variables at first point of use, rather than at the beginning of the function (C style.)

Other than the bug and (a) too many cin.get()s and (b) no need for <string>, the rest of my comments are about how to make the program even better, rather than actual corrections. (In my opinion, of course...)

So:

a. variable names should always make it clear what the value is, e.g. x -> bmi.
b. function names, as a general rule, should say what they do, e.g. bmi() -> calcBmi(). [i]
b. could you use a little less vertical space with losing clarity? It's not so bad here, as it's a short program, but the display screen is only so high. You should keep associated lines grouped together, and groups separated.
c. is the program as polite as it could be? ("Height in inches" ?)
d. have you init all variables? (good form)
e. you should preferably initialize a variable on the same line as you declare them (combine lines 35 and 37)

And ideally, you should avoid using using namespace std; at file scope, though it's ok in a small program like this.

Andy

[i] The exceptions are function that get/set a property, which are named after the property in some case, relying on operator overloading to distinguish the get and set forms (as used in C++ standard library, e.g. ios_base::width has these forms:

streamsize width() const;
streamsize width (streamsize wide);