3
\$\begingroup\$

The following question was taken from Absolute Java 5th ed. by Walter Savitch:

A simple rule to estimate your ideal body weight is to allow 110 pounds for the first 5 feet of height and 5 pounds for each additional inch. Write a program with a variable for the height of a person in feet and another variable for the additional inches. Assume the person is at least 5 feet tall. For example, a person that is 6 feet and 3 inches tall would be represented with a variable that stores the number 6 and another variable that stores the number 3. Based on these values, calculate and output the ideal body weight.

This is the code that I have written:

 public class Question9 {

    private static final int INCH_PER_FEET = 12;
    private static final int POUND_PER_INCH = 5;
    private static final int MIN_HEIGHT_FEET = 5;
    private static final int MIN_WEIGHT_POUND = 110;

    public static void main(String[] args) {

        int feet = 6;
        int inches = 3;

        System.out.println(idealBodyWeight(feet, inches) + " pounds");
    }

    private static int idealBodyWeight(int feet, int inches) {
        int pounds = MIN_WEIGHT_POUND;

        if ((feet > MIN_HEIGHT_FEET)) {

            inches += (feet - MIN_HEIGHT_FEET) * INCH_PER_FEET;
        }

        pounds += inches * POUND_PER_INCH;

        return pounds;
    }
}
\$\endgroup\$
  • \$\begingroup\$ The if is unnecessary, and the println could be replaced by a printf. Otherwise, looks pretty good to me. \$\endgroup\$ – raptortech97 Sep 13 '14 at 16:27
5
\$\begingroup\$

There isn't really all that much to say, your code is generally good.

  • Declare variables where they are needed: you do not use pounds in the if statement, so I would remove it and just use MIN_WEIGHT_POUND in the return directly. Alternatively, you could move the declaration of pounds to after the if.
  • Try to minimize white space: It's good to sort code by using newlines, but too much isn't good either. remove the newline after the if statement, and maybe some more.
  • Try to minimize use of brackets: No need to wrap the boolean expression in brackets.

With these changes, your code would look like this:

private static int idealBodyWeight(int feet, int inches) {
    if (feet > MIN_HEIGHT_FEET) {
        inches += (feet - MIN_HEIGHT_FEET) * INCH_PER_FEET;
    }
    return MIN_WEIGHT_POUND + inches * POUND_PER_INCH;
}
\$\endgroup\$
  • \$\begingroup\$ I don't think you meant "brackets". \$\endgroup\$ – IEatBagels Sep 13 '14 at 23:54
  • \$\begingroup\$ @TopinFrassi english isn't my first language, so I'm not sure. But looking at wikipedia, it seems that brackets would be the correct term? \$\endgroup\$ – tim Sep 14 '14 at 11:26
  • \$\begingroup\$ Well if you talk about these => (). They are parenthesis. But I'm not sure about the spelling \$\endgroup\$ – IEatBagels Sep 14 '14 at 14:30
4
\$\begingroup\$

Assume the person is at least 5 feet tall.

The code doesn't handle this assumption well: the function will return a value even if the height is less than 5 feet, without signaling to the user that the value might not be correct, since we don't have specification for this case. Instead of quietly ignoring this and returning some potentially incorrect value, it would be better to raise an error.


The method parameter names are not very good:

private static int idealBodyWeight(int feet, int inches) {

feet doesn't tell that it's actually a "height", and that inches are the additional inches on top of the height in inches. This makes the method hard to read.


It's not recommended to change the values of parameter variables:

inches += (feet - MIN_HEIGHT_FEET) * INCH_PER_FEET;

It would be better to create a new local variable instead, for example:

int totalInches = inches;
totalInches += (feet - MIN_HEIGHT_FEET) * INCH_PER_FEET;

The pounds local variable seems quite pointless. You can rewrite the code much simpler without it.

Suggested implementation

private static int calculateIdealBodyWeight(int heightInFeet, int additionalInches) {
    if (heightInFeet < 5) {
        throw new IllegalArgumentException("height in feet must be >= 5");
    }
    int inches = (heightInFeet - MIN_HEIGHT_FEET) * INCH_PER_FEET + additionalInches;
    return MIN_WEIGHT_POUND + inches * POUND_PER_INCH;
}
\$\endgroup\$

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.