2
\$\begingroup\$

This is my first program so no doubt I've made mistakes. The code works and meets all requirements as far as I can see.

(Further info: program to calculate Body Mass Index (BMI) allowing user to enter weight in stones and pounds or kilograms It allows user to enter height in feet and inches or centimeters.

It then calculates BMI and displays the result.

I'm hoping to get a critical review of the code to identify any potential problems with it. I am open to any criticism and any suggestions on how I can improve the code and improve the overall quality.

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

public class BmiCalculatorWorking
{      
    static double totalIn;
    static double kgs;
    static double totalPound;
    static double pounds;
    static int stone;
    static int feet;
    static int inches;
    static double cms;
    static double totalCm;

       public static void main(String[] args)
       {
           Scanner input = new Scanner( System.in );
           boolean continueLoop = true;
               do
                 {
                     try
                     {
                     System.out.println("Welcome to the BMI Calculator!\nYou"+
                             " may calculate your BMI in 1 of 2 ways:\n\n1. by"+
                             " entering Weight in stone and pounds and Height"+
                             " in feet and inches (Imperial) \nor\n2. by"+
                             " entering Weight in Kilograms and Height"+
                             " in Meters (Metric)");

                     while (true)
                     {
                     System.out.print("\nEnter 1 for Imperial OR Enter 2 for"+
                                      " Metric: ");

                     int userInput;
                     userInput = input.nextInt();

                        if (userInput == 1)
                        {  
                        bmiCalcImperial();  

                        break;
                        }
                           else if (userInput == 2)
                           {    
                           bmiCalcMetric();
                           break;
                           }
                              else
                              {
                              System.out.println("Invalid Input, please"+
                                     " enter either 1 or 2");
                              continue;
                              }      
                      }
                      continueLoop = false;
                      }
                  catch ( InputMismatchException inputMismatchException)
                  {
                     System.err.printf( "\nException: %s\n", inputMismatchException );
                     input.nextLine();
                     System.out.println("Input invalid! You must enter"+
                                        " numbers only. Please try again.\n" );
                  }
              }while ( continueLoop );                  
       }

           public static void bmiCalcImperial()
           {
           Scanner input = new Scanner( System.in );
           boolean continueLoop = true;
        //ENTER WEIGHT IN STONE AND/OR POUNDS within range AND CONVERT TO KGS
     do{
        System.out.print("\nPlease enter your weight in Stone: ");
           try{
               stone = input.nextInt();

                  while (stone < 3 || stone > 30)
                  {
                  System.out.print("Please enter a valid weight (between 3 and"+
                                   " 30 stone):");
                  stone = input.nextInt();
               }
           continueLoop = false;
               }
               catch ( InputMismatchException inputMismatchException)
                  {
                   System.err.printf( "\nException: %s\n", inputMismatchException );
                   input.nextLine();
                   System.out.println("Input invalid! You must enter"+
                                      " (St)numbers only. Please try again.\n" );
                  }

        }while ( continueLoop ); 

        continueLoop = true;
      do{
         System.out.print("Please enter your weight in Pounds: ");
         try  {
              pounds = input.nextDouble();

                 while (pounds < 0 || pounds > 400)
                 {
                 System.out.print("Please enter a valid weight"+
                                  " (between 0 and 400 pounds):");
                 pounds = input.nextDouble();
                 }
             continueLoop = false; 
              }
           catch ( InputMismatchException inputMismatchException)
                 {
                 System.err.printf( "\nException: %s\n", inputMismatchException );
                 input.nextLine();

                 System.out.println("Input invalid! You must enter"+
                                 " (Ibs)numbers only. Please try again.\n" );
                 }

         }while ( continueLoop ); 

           totalPound = stone*14 + pounds;
           kgs = totalPound*0.45359237;
           System.out.printf("Your weight in Kilograms is: %.3f", kgs);
           System.out.print(" Kgs\n");

         continueLoop = true;
           //ENTER HEIGHT IN FEET AND/OR INCHES within range AND CONVERT TO CMS 
        do{
            try{
               System.out.print("\nPlease enter your Height in Feet: ");
               feet = input.nextInt();

                  while (feet < 3 || feet > 7)
                  {
                  System.out.print("Please enter a valid Height (between"+
                                   " 3 and 7 Feet):");
                  feet = input.nextInt();
                  }
             continueLoop = false; 
                }
              catch ( InputMismatchException inputMismatchException)
                   {
                   System.err.printf( "\nException: %s\n", inputMismatchException );
                   input.nextLine();
                   System.out.println("Input invalid! You must enter (FEET)"+
                                      "numbers only. Please try again.\n" );
                   }          
            }while ( continueLoop );

         continueLoop = true;
       do{
          try{
             System.out.print("Please enter your Height in Inches: ");
             inches = input.nextInt();

                while (inches < 0 || inches > 84)
                 {
                 System.out.print("Please enter a valid Height (between"+
                                  " 0 and 84 inches):");
                 inches = input.nextInt();
                  }
         continueLoop = false; 
              }
            catch ( InputMismatchException inputMismatchException)
                  {
                  System.err.printf( "\nException: %s\n", inputMismatchException );
                  input.nextLine();
                  System.out.println("Input invalid! You must enter"+
                                     " (Inches)numbers only. Please try again.\n" );
                  }          
          }while ( continueLoop );
           //converts feet and Inches to total Inches and then converts to centimetres
           totalIn = feet*12 + inches;
           totalCm = totalIn*2.54;
           System.out.printf("Your height in Centimeters is: %.0f", totalCm);
           System.out.print(" cms\n");

          //Calculate BMI by converting from Imperial user input to metric
          //and then using the metric formula

                double ImperialToMetricBmi;
                double InchesToMtrHeight;

                InchesToMtrHeight = totalIn*2.54/100;
                ImperialToMetricBmi = (kgs/(InchesToMtrHeight*InchesToMtrHeight));

                 System.out.printf("\nYour (converted to use Metric"+
                                   " formula)  BMI is: %.3f\n" ,ImperialToMetricBmi);
           //Display BMI description based on Imperial user input
           //calcualted with metric formula
           if(ImperialToMetricBmi < 18.5)
             {
              System.out.println("You are underweight and are at risk of "+
                      "developing problems such as nutritional deficiency and "+
                      "osteoporosis.\nIt is recommended you seek professional"+
                      " medical advice.");
             }
                 else
                      if (ImperialToMetricBmi >= 18.5 && ImperialToMetricBmi <= 24.9)
                         {        
                         System.out.println("You are a healthy, normal weight");
                         }
                    else
                          if (ImperialToMetricBmi >= 25 && ImperialToMetricBmi <= 29.9)
                             {
                             System.out.println("You are overweight and are at"+""
                                    + " moderate risk of developing"+"\n"
                                    + " heart disease, high blood pressure,"+
                                    " stroke, diabetes.\n It is recommended "+
                                    "you lose weight through exercise and"+
                                    " a healthy diet.");
                             }
                       else
                            if (ImperialToMetricBmi > 30 )
                               {
                               System.out.println("You are obese and are at"+
                                       " High risk of developing heart"+
                                       " disease, high blood pressure,"+
                                       " stroke, diabetes. It is recommended"+
                                       " you seek professional"+
                                       " medical advice.    ");
                               }
           }          
              public static void bmiCalcMetric()
              {
              Scanner input = new Scanner(System.in);
              boolean continueLoop = true;
              //ENTER WEIGHT IN KGS AND CONVERT TO STONE AND POUNDS
              double mtr;
              double bmi;

                 do{       
                   try{       
                   System.out.print("\nPlease enter your weight in Kilograms: ");
                   kgs = input.nextDouble();

                         while (kgs < 20 || kgs > 180)
                         {
                         System.out.print("Please enter a valid weight "+
                                          "(between 20 and 180kgs):");
                         kgs = input.nextDouble();
                         }


                         continueLoop = false;
                      }
                catch ( InputMismatchException inputMismatchException)
                      {
                      System.err.printf( "\nException: %s\n", inputMismatchException );
                      input.nextLine();
                      System.out.println("Input invalid! You must enter (KGS)"+
                                         "numbers only. Please try again.\n" );
                      }

                    }while (continueLoop);        
              //note: Converts kgs to ibs **But not St AND Ibs !!**
              pounds = kgs*2.204622;
              System.out.printf("Your weight in pounds is: %.0f" ,pounds);
              System.out.print(" Ibs\n");
              continueLoop = true;
       do{          
           try{
               System.out.print("\nPlease enter your Height in Centimeters: ");
               cms = input.nextInt(); //should this be .nextDouble?

                   while (cms < 91 || cms > 213)
                   {
                   System.out.print("Please enter a valid Height (between"+
                                    " 91 and 213cms):");
                   cms = input.nextDouble();
                   }
               continueLoop = false;
               }
         catch ( InputMismatchException inputMismatchException)
               {
               System.err.printf( "\nException: %s\n", inputMismatchException );
               input.nextLine();
               System.out.println("Input invalid! You must enter (cms)"+
                                  "numbers only. Please try again.\n" );
               }
         }while (continueLoop);               

              totalIn = cms/2.54;
              System.out.printf("\nYour height in inches is: %.0f" ,totalIn);
              System.out.print( "\" \n");

              //Calculate BMI
              mtr = cms/100;
              bmi = (kgs/(mtr*mtr));
              //Display BMI
              System.out.printf("\nYour BMI is: %.3f\n" ,bmi);

              //Display BMI description
              if(bmi < 18.5)
              {
              System.out.println("\nYou are underweight");
              }
                 else
                      if (bmi >= 18.5 && bmi <= 24.999)
                         {        
                         System.out.println("\nYou are a healthy, normal weight");
                         }
                    else
                         if (bmi >= 25 && bmi <= 29.999)
                            {
                            System.out.println("\nYou are overweight");
                            }
                       else
                            if (bmi > 30 )
                               {
                               System.out.println("\nYou are obese");
                               }

              }
}
\$\endgroup\$
  • 6
    \$\begingroup\$ Hi Julie, please include more context in your post. Also, maybe you've had some problem with indenting your code when posting it here? Try to fix it if that's not how you see code in your IDE. Otherwise surely a review element would be code indentation :) \$\endgroup\$ – IEatBagels Oct 24 '19 at 17:23
  • 1
    \$\begingroup\$ I'm assuming BMI means Body Mass Index, but I could be wrong. That is probably what @IEatBagels is referring to. Please describe it in a little more detail. \$\endgroup\$ – pacmaninbw Oct 24 '19 at 18:44
  • 2
    \$\begingroup\$ @IEatBagels Post edited to include full context of what the program does. Thanks for mentioning Indentation, I thought it was correct(ish) but is it indented too much? \$\endgroup\$ – Julie Cohen Oct 24 '19 at 21:27
  • 1
    \$\begingroup\$ @pacmaninbw Yes, Body Mass Index, I've edited my post to include all details of what the program does. Thanks! \$\endgroup\$ – Julie Cohen Oct 24 '19 at 21:29
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Oct 25 '19 at 20:11
4
\$\begingroup\$

BmiCalculatorWorking This name is confusing. All code should eventually be 'working'.

Don't use static for all variables. I can only guess you did this to avoid the warning about calling non-static variables from a static method.

Instead, you can instantiate a BmiCalculatorWorking, and call the methods on the object.

You should never use while(true), instead put the condition inside the while loop.

If you've learned how to create methods, you should start practicing making them. It'll make your code much easier to read & much easier to refactor. For example, printing out the welcome message could be a method.

This can be shortened to: int userInput = input.nextInt();

int userInput;
userInput = input.nextInt();

bmiCalcImperial is a really bad name for a method. It's not descriptive. This method is also doing waaaaay too much. Methods should only do 1 thing. This becomes easy when you name your methods descriptively. For example, this method would have a really long & awkward name.

continueLoop is a bad name. Try to give descriptive names. Naming it continueLoop because it's used in a while statement is silly.

Just want to further emphasis that breaking this down into methods would substantially increase readability. Even for yourself. For example when trying to figure out your own errors or add functionality.

Indentation also plays a huge part in readability. An IDE will help you with this but I understand some college/university courses start off disallowing an IDE and I will assume that's the case here.

You shouldn't shorten variables names. bmi is okay, but I can't think of what cms, mtr stand for. Even if it's obvious, the computer doesn't care about how long your variable names are and you aren't saving much time by shortening them. It's not a big deal here but something to keep in mind for future programs.

\$\endgroup\$
  • 3
    \$\begingroup\$ Just want to further emphasis that breaking this down into methods would substantially increase readability. Yes, that's an important critique of this code. Writing one big blob of code like this is an anti-pattern (but also common for beginners). \$\endgroup\$ – markspace Oct 24 '19 at 23:15
  • \$\begingroup\$ @dustytrash All advice taken into account I've spent many hours researching your comments and have made a lot of changes -I'll change the name -I think I've instantiated to call methods -I don't know any other way but {while(true)} Can't figure out how to put inside loop. Can you give an example? -I now have 9 methods instead of 3. bmiCalcImperial now split into 3 methods -What would be a better name than {continueLoop}? -I've changed var names to be clear \$\endgroup\$ – Julie Cohen Oct 25 '19 at 19:57
  • \$\begingroup\$ I'm still confused about static/non-static methods but my revised code works (apart from the int = double*2.2 problem which I am working on now). Will I enter my revised code above or should I create a new post as it's very different? Thank you all. \$\endgroup\$ – Julie Cohen Oct 25 '19 at 19:58
1
\$\begingroup\$

Dependency Management

The Scanner is a dependency in each of the 3 methods and may be difficult to unit test around.

Magic Numbers

Explicit numbers used should be wrapped in a constant to better explain its purpose. 0.45359237 is an example of this. It is difficult to understand what some of these numbers are used for unless you are deep in the implementation details.

Object-Oriented Composition Opportunity

We aren't really using object-oriented tools provided by Java. There is only one class used in a very procedural way.

I created a design to help practice object-oriented composition. Instead of one main class calling two methods within your single calculator class, my design creates three new classes. One base class called BMICalculation. Then two more specific subclasses that inherit from that base class called ImperialBMICalculation and MetricBMICalculation. In the constructor for the two subclasses, you will put the user prompt to get the height and weight information for the calculation of each type. Also during construction, you can calculate the BMI for the object and use it to populate a results string on each object. BMICalculation will have methods that will be overridden by the subclasses such as displaying the result of a particular calculation. This gives you the option to create many instances of BMI calculation objects or be able to attach the object to something like a person object.

Conclusion

Great start! I like the exception handling and attention to detail. I hope this feedback helps and I will leave my live code review below.

Live code review: https://youtu.be/RMJWdH2dQXU

\$\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.