5
\$\begingroup\$

A similar question must have been asked a thousand times, but for the life of me I can't figure this out. I am new to Java and I want to create a programme that asks a user for his or her height and weight, and calculate the corresponding BMI. I want to throw exceptions for the wrong input (has to be a double, non-negative and not zero) and I want to catch any mathematical errors that may occur. The follow code does that, but I have exact duplication for handling the input of height and weight. How can I simplify this into a more concise format without losing any functionality?

import java.util.InputMismatchException;
import java.util.Scanner;

public class BMIcalculator {
    public static void main(String[] args){
        double BMI = calculateBMI();
        System.out.print("\nYour BMI is " + BMI + ".");
    }

    public static double calculateBMI() {
        // Scanner allows us to request a users input
        Scanner scan = new Scanner(System.in);
        // Initialize doubles as zero values
        double w=0, h=0;

        // Ask for user input (weight)
        System.out.println("Please enter your weight");
        while(true) {
            try {
                w = scan.nextDouble();
                if (w <= 0) throw new ArithmeticException();
                break;
            } catch (InputMismatchException ime) {
                System.err.println("Ahw, error: " + ime.getMessage());
                continue;
            } catch (ArithmeticException ae) {
                System.err.println("Weight can't be less than or equal to zero!");
                continue;
            }
        }
        // Ask for user input (height)
        System.out.println("Please enter your height");
        while(true) {
            try {
                h = scan.nextDouble();
                if (h <= 0) throw new ArithmeticException();
                break;
            } catch (InputMismatchException ime) {
                System.err.println("Ahw, error: " + ime.getMessage());
                continue;
            } catch (ArithmeticException ae) {
                System.err.println("Height can't be less than or equal to zero!");
                continue;
            }
        }
        // try-catch not necessary; we only get here when our input is valid


        // we have to close the input connection to the user
        scan.close();
        return w / (h * h);
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ When you call scan.close(), it closes the underlying stream (System.in), so you can't re-create a scanner on System.in after that. You need to create the scanner in main and pass it on to your input function, closing it only once in your program. Haven't done Java in ages, but that ArithmeticException makes me cringe. Don't use exceptions for control flow. \$\endgroup\$ – Mat Oct 26 '15 at 14:04
  • \$\begingroup\$ @Mat To be honest, I'm following a course on Java at uni and I'm just solving some puzzles to get better at it. I do follow directions (slides), so mabe there's something wrong in the slides or I'm misinterpreting them. I suppose that you mean that I can't just use ArithmeticException as some sort of "variable exception" that I can throw whenever I please? How else should I do this? \$\endgroup\$ – Bram Vanroy Oct 26 '15 at 17:24
  • 1
    \$\begingroup\$ See e.g. here: programmers.stackexchange.com/questions/189222/… (see also the linked questions on that page). Use a plain if/else in this specific case. h.j.k has the best option in the answers you got, IMO. \$\endgroup\$ – Mat Oct 26 '15 at 18:07
5
\$\begingroup\$

Avoiding code repetition

As pointed out in some of the other answers, getting a double value from the keyboard can be abstracted into its own method to avoid code repetition:

private static double getDouble(Scanner scanner, String metric) {
    System.out.println("Enter " + metric);
    double result;
    while (!scanner.hasNextDouble() || (result = scanner.nextDouble()) <= 0) {
        System.out.println(metric + " must be a positive number.");
        scanner.next();
    }
    return result;
}

try-with-resources

If you are on Java 7 and above, you should use try-with-resources for efficient handling of the underlying I/O resource:

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) {
        double weight = getDouble(scanner, "Weight (kg)");
        double height = getDouble(scanner, "Height (m)");
        System.out.println("Your BMI is: " + /* calculation */);
    }
}

Units

It's important to note that BMI is expressed in units of \$\frac{kg}{m^2}\$, so you should definitely display the units of measurement too. Otherwise, you'll need extra methods to perform the conversion to kilograms and meters. :)

Comments

Simply put, comments should explain the why, not how. Most of your comments now are just descriptive, i.e. on the how, which is already apparent in the code. Comments are sometimes seen as redundant as they are prone to being outdated. Therefore, comment sparingly, and only when you really need it to explain something that is non-obvious. Even so, try to make the code 'obvious' first. :)

\$\endgroup\$
  • \$\begingroup\$ Thank you for the many tips! I'm getting a course on Java so I'm simply a beginner, but this is very useful. I don't have much time now, but I'll get to testing in the next couple of days. Could you explain the condition in the while-loop? I don't understand what it means in human language. :-) \$\endgroup\$ – Bram Vanroy Oct 26 '15 at 17:29
  • 2
    \$\begingroup\$ The trick in the second half of the while loop is that the expression result = scanner.nextDouble() actually has a result. It evaluates to the value of the assignment, which is the same thing as what winds up in result. So the <= 0 is checking if result is <= 0, in which case it's prompting the user again. \$\endgroup\$ – Eric Stein Oct 26 '15 at 18:03
2
\$\begingroup\$

I'm not a java programmer so this might not be right.

The weight and height code are almost the same so could you try:

public static double getANumber (String szThing)
{
    double result = 0.0;
    // Ask for user input
    System.out.println("Please enter your " + szThing);
    while(true) {
        try {
            result = scan.nextDouble();
            if (result <= 0) throw new ArithmeticException();
            break;
        } catch (InputMismatchException ime) {
            System.err.println("Ahw, error: " + ime.getMessage());
            continue;
        } catch (ArithmeticException ae) {
            System.err.println(szThing + " can't be less than or equal to zero!");
            continue;
        }
    }
\$\endgroup\$
  • \$\begingroup\$ Thanks. I tried this, slightly modified, but I get a NoSuchElementException exception. Please see my edit in my first post. \$\endgroup\$ – Bram Vanroy Oct 26 '15 at 13:10
  • \$\begingroup\$ I like the mods, it looks much more java like. I'm having to guess at this, but is h=getBMIValue("height") line 15? If so I would guess it is something to do with scan. Can you move the creation and closure of scan outside the function and pass it as a parameters? \$\endgroup\$ – Code Gorilla Oct 26 '15 at 14:00
  • \$\begingroup\$ @BramVanroy just add a closing } and pass the scan as a parameter \$\endgroup\$ – Caridorc Oct 26 '15 at 15:10
1
\$\begingroup\$

Change the while loops to this. Check the scanner to see if their is the correct input ready -> if not tell them its wrong and skip to the next line. If it is a number, check it to make sure its > 0. If its <= 0 then we set the w back to 0 so the loop tries again.

while (w == 0) {
    if(scan.hasNextDouble()){
        w = scan.nextDouble();
        if(w <= 0){
            System.err.println("Weight can't be less than or equal to zero!");
            scan.nextLine();
            w = 0;
         }
    }else {
         System.err.println("Please enter a number!");
         scan.nextLine();
    }
}

Using exceptions for this is not the best way to find errors, unless you have been told to do that way, this is a better solution (I think?)

import java.util.Scanner;

public class BMIcalculator {

    public static void main(String[] args) {
        BMIcalculator bmi = new BMIcalculator();
        double BMI = bmi.calculateBMI();
        System.out.print("\nYour BMI is " + BMI + ".");
    }

    public double calculateBMI() {
        // Scanner allows us to request a users input
        Scanner scan = new Scanner(System.in);
        // Initialize doubles as zero values
        double w = 0, h = 0;

        // Ask for user input (weight)
        System.out.println("Please enter your weight");
        while (w == 0) {
            if(scan.hasNextDouble()){
                w = scan.nextDouble();
                if(w <= 0){
                    System.err.println("Weight can't be less than or equal to zero!");
                    scan.nextLine();
                    w = 0;
                }
            }else {
                System.err.println("Please enter a number!");
                scan.nextLine();
            }
        }
        // Ask for user input (height)
        System.out.println("Please enter your height");
        while (h == 0) {
            if(scan.hasNextDouble()){
                h = scan.nextDouble();
                if(h <= 0){
                    System.err.println("Height can't be less than or equal to zero!");
                    scan.nextLine();
                    h = 0;
                }
            }else {
                System.err.println("Please enter a number!");
                scan.nextLine();
            }
        }
        // try-catch not necessary; we only get here when our input is valid

        // we have to close the input connection to the user
        scan.close();
        return w / (h * h);
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Good point with the exceptions, now you could also refactor the duplicate loop into a separate method like the other answers do, so we get the best of both. \$\endgroup\$ – ferada Oct 26 '15 at 15:07
  • 1
    \$\begingroup\$ I only have a bit of time, so I am quickly going over the answers here but indeed, we've been told to solve these set of questions with try-catch and exceptions. My instinct was to use if-statements as well but that wasn't allowed. :( \$\endgroup\$ – Bram Vanroy Oct 26 '15 at 17:26
  • \$\begingroup\$ This question is similar and has multiple answers using the try/catch. \$\endgroup\$ – Eric G Oct 26 '15 at 19:27
1
\$\begingroup\$

Having the main method within your logic class is never good. Create something like an Application class and keep the main method there! :)

Then, notice you have 3 steps, which are :

  1. Ask for weight
  2. Ask for height
  3. Compute

I feel like this could use at least two methods. ReadWeight and ReadHeight. Now, what does this look like :

public class Application {
    public static void main(String[] args){
        double BMI = BMIcalculator.calculate();
        System.out.print("\nYour BMI is " + BMI + ".");
    }
}

public class BMIcalculator {

    public static double calculate() {
        Scanner scan = new Scanner(System.in);
        double weight= readWeight(scan);
        double height = readHeight(scan);

        scan.close();
        return weight / (height * height);
    }

    private static double readWeight(Scanner scanner) {
        System.out.println("Please enter your weight");
        while(true) {
            try {
                double weight = scan.nextDouble();
                if (weight <= 0) throw new ArithmeticException();
                return weight;
            } catch (InputMismatchException ime) {
                System.err.println("Ahw, error: " + ime.getMessage());
                continue;
            } catch (ArithmeticException ae) {
                System.err.println("Weight can't be less than or equal to zero!");
                continue;
            }
        }
    }

    private static double readHeight(Scanner scanner) {
        System.out.println("Please enter your height");
        while(true) {
            try {
                double height = scan.nextDouble();
                if (height <= 0) throw new ArithmeticException();
                return height;
            } catch (InputMismatchException ime) {
                System.err.println("Ahw, error: " + ime.getMessage());
                continue;
            } catch (ArithmeticException ae) {
                System.err.println("Height can't be less than or equal to zero!");
                continue;
            }
        }
    }
}

Notice I've changed h and w for height and weight. It's clearer!

One could argue that readHeight and readWeight should be in the same method, and have slightly different parameters to decide which String to show, but I think it's best to keep them separated. After all, maybe you'll end up asking for a feet/foot height as "5'1''" instead of inches. That way your methods can change their logic without impacting the other one.

\$\endgroup\$
  • \$\begingroup\$ @ferada Ehm, are you sure? Maybe it's something I don't see, but isn't readHeight the third function in the class? \$\endgroup\$ – IEatBagels Oct 26 '15 at 15:10
  • \$\begingroup\$ Could be my internet, it's there now. \$\endgroup\$ – ferada Oct 26 '15 at 15:30

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.