BMI Calculator Java Swing Practice

Question

This is my first question here and I'd like to get some feedback. The program works but I'm not sure the classes I've used are right.

First, a class to calculate the Body Mass Index from a double (kgs) and an integer (height in cm):

//Data taken from https://codereview.stackexchange.com/questions/216933/java-swing-exercise-bmi-calculator
//NavigableMap idea taken from https://codereview.stackexchange.com/questions/142708/school-grading-string-calculator/142752#142752
import java.util.Map;
import java.util.NavigableMap;
import java.util.TreeMap;

public class PersonHealthData {
  private final double weight; //Kg.
  private final int height; //cm.

  private static final NavigableMap<Double, String> grades = new TreeMap<>();

  static {
    grades.put(0.0, "Insuficient Weight");
    grades.put(18.5, "Normal Wight");
    grades.put(24.9, "Overweight Grade 1");
    grades.put(26.9, "Overweight Grade 2: Probesity");
    grades.put(29.9, "Obesity Grade 1");
    grades.put(34.9, "Obesity Grade 2");
    grades.put(39.9, "Obesity Grade 3: Morbid");
    grades.put(49.9, "Obesity Grade 4: Extreme");
  }


  public PersonHealthData(double weight, int height) {
    this.weight = weight;
    this.height = height;
  }

  public double getWeight() {
    return weight;
  }

  public int getHeight() {
    return height;
  }

  public double getBMI() {
    double heightMeters = height / 100.0;
    return weight / (double) (heightMeters * heightMeters);
  }

  public String getGrade() {
    Map.Entry<Double, String> gradeEntry = grades.floorEntry(getBMI());
    if (gradeEntry == null) {
      return "Incorrect Data";
    }
    return gradeEntry.getValue();
  }

}

The second class is the Main program (creates the interface and acts like a controller I guess)

import javax.swing.*;
import java.awt.*;

public class MainProgram {
  private final JFrame window;
  private final EntryPanel dataEntry;
  private final ResultPanel results;

  MainProgram() {
    window = new JFrame("BMI CAlculator");
    window.setLayout(new GridLayout(0, 1));
    dataEntry = new EntryPanel(this);
    window.add(dataEntry.getPanel());

    results = new ResultPanel();
    window.add(results.getPanel());

    window.pack();
    window.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
    window.setLocationRelativeTo(null);
  }

  public void show() {
    window.setVisible(true);
  }

  public static void main(String[] args) {
    MainProgram mainProgram;
    mainProgram = new MainProgram();
    mainProgram.show();

  }

  public void action() {
    double weight;
    try {
      weight = Double.parseDouble(dataEntry.getStringWeight());
    } catch (NumberFormatException e) {
      results.error("Weight input is not a number");
      return;
    }

    int height;
    try {
      height = Integer.parseInt(dataEntry.getStringHeight());
    } catch (NumberFormatException e) {
      results.error("Height input is not a number");
      return;
    }
    PersonHealthData data = new PersonHealthData(weight, height);
    results.setResults(String.format("Your BMI is: %.2f", data.getBMI()), data.getGrade());

  }
}

And two classes to practice Swing components and listeners. The first one is an input Panel:

import javax.swing.*;
import javax.swing.border.EmptyBorder;
import java.awt.*;
import java.awt.event.KeyAdapter;
import java.awt.event.KeyEvent;

public class EntryPanel {
  private final JPanel panel;
  private final JPanel panelSliders;
  private final JPanel panelInput;

  private final JTextField textWeight;
  private final JTextField textHeight;
  private final MainProgram mainProgram;
  private final JSlider sliderWeight;
  private final JSlider sliderHeight;

  EntryPanel(MainProgram mainProgram) {
    this.mainProgram = mainProgram;
    panel = new JPanel(new GridLayout(0, 1));
    panelSliders = new JPanel(new GridLayout(0, 2));
    sliderWeight = new JSlider();
    sliderWeight.setMinimum(10);
    sliderWeight.setMaximum(200);
    panelSliders.add(sliderWeight);
    panelSliders.add(new JLabel("Weight"));

    sliderHeight = new JSlider();
    sliderHeight.setMinimum(10);
    sliderHeight.setMaximum(270);
    panelSliders.add(sliderHeight);
    panelSliders.add(new JLabel("Height"));
    panelSliders.setBorder(new EmptyBorder(10, 10, 10, 10));
    panel.add(panelSliders);

    panelInput = new JPanel(new GridLayout(0, 3, 10, 10));
    panelInput.setBorder(new EmptyBorder(10, 10, 10, 10));
    panelInput.add(new JLabel("Weight"));
    textWeight = new JTextField();
    textWeight.addKeyListener(new KeyAdapter() {
      @Override
      public void keyReleased(KeyEvent e) {
        super.keyReleased(e);
        mainProgram.action();
      }
    });
    textWeight.setHorizontalAlignment(SwingConstants.RIGHT);
    panelInput.add(textWeight);
    panelInput.add(new JLabel("kg."));
    panelInput.add(new JLabel("Height"));
    textHeight = new JTextField();
    textHeight.setHorizontalAlignment(SwingConstants.RIGHT);
    textHeight.addKeyListener(new KeyAdapter() {
      @Override
      public void keyReleased(KeyEvent e) {
        super.keyReleased(e);
        mainProgram.action();
      }
    });
    panelInput.add(textHeight);
    panelInput.add(new JLabel("cm."));
    sliderWeight.addChangeListener(e -> {
      textWeight.setText("" + sliderWeight.getValue());
      mainProgram.action();
    });
    sliderHeight.addChangeListener(e -> {
      textHeight.setText("" + sliderHeight.getValue());
      mainProgram.action();
    });
    panel.add(panelInput);
  }

  public JPanel getPanel() {
    return panel;
  }

  public String getStringWeight() {
    return textWeight.getText();
  }

  public String getStringHeight() {
    return textHeight.getText();
  }
}

And the second one is used to show the results:

import javax.swing.*;
import javax.swing.border.EmptyBorder;
import java.awt.*;

public class ResultPanel {

  private final JPanel panel;
  private final JLabel BMI;
  private final JLabel information;

  ResultPanel() {
    panel = new JPanel();
    panel.setLayout(new GridLayout(0, 1, 10, 10));
    panel.setBorder(new EmptyBorder(10, 10, 10, 10));
    BMI = new JLabel("Enter your weight and height in centimeters.");
    BMI.setHorizontalAlignment(SwingConstants.CENTER);
    information = new JLabel("");
    information.setHorizontalAlignment(SwingConstants.CENTER);
    panel.add(BMI);
    panel.add(information);
  }

  public JPanel getPanel() {
    return panel;
  }

  public void setResults(String IMC, String information) {
    this.BMI.setText(IMC);
    this.BMI.setForeground(Color.BLACK);
    this.information.setText(information);
  }

  public void error(String s) {
    this.BMI.setText(s);
    this.BMI.setForeground(Color.RED);
    this.information.setText("");
  }
}

No Unit Tests because We've not gone so far in my class.

Any advice or comment will be very appreciated and welcome.

Answer 1

Java naming conventions state that variables and methods (including fields/members) should be lowerCamelCase. If they contain an acronym or similar, like HTTP, it should only have the leading letter uppercase when not at the start of the name, like "httpClient" or "performHttpAction".

Naming conventions are an important tool to let somebody quickly know that they are looking at. 99.9999% of Java programmers will immediately assume the name "BMI" to belong a constant of some sort.

Last but not least, you're using "BMI" throughout the whole application. If you can, you should avoid domain specific abbreviations as they might not be completely understandable when somebody enters the domain.

---

private final double weight; //Kg.
private final int height; //cm.

Javadoc. Or even better, make it part of the variable name weightInKg. Or even better, create a domain specific type.

---

private static final NavigableMap<Double, String> grades = new TreeMap<>();

As a note, even though this is declared final, the content can still be changed.

Also, maybe this should come from a configuration in the end.

---

grades.put(0.0, "Insuficient Weight");

Autoboxing.

---

grades.put(18.5, "Normal Wight");

Typo.

---

  public double getBMI() {
    double heightMeters = height / 100.0;
    return weight / (double) (heightMeters * heightMeters);
  }

Cache the BMI (calculate it once in the construcor), it will not change.

  public String getGrade() {
    Map.Entry<Double, String> gradeEntry = grades.floorEntry(getBMI());
    if (gradeEntry == null) {
      return "Incorrect Data";
    }
    return gradeEntry.getValue();
  }

Same here.

---

Short talk on API design, your class currently looks like this:

public class PersonHealthData {
  private final double weight; //Kg.
  private final int height; //cm.

  private static final NavigableMap<Double, String> grades = new TreeMap<>();

  public double getWeight();
  public int getHeight();
  public double getBMI();
  public String getGrade();
}

So, let's assume I want to extend this class. There is very little wiggle room with the values being private and final. However, getWeight() and getHeight() are not final. So if I want to return a different weight, for example because I want the class to be mutable, I'd implement a setter, have another private member holding my new weight and override getWeight(). What now happens is that getBMI and getGrade will return incorrect values, unless I re-implement them, however, their logic might not be known to me. Also, I might not quickly notice that problem.

Possible solutions:

1. Make the class final.
2. Make getWeight and getHeight final. This removes effectively any possibility to extend the class.
3. Make weight and height protected and not final. That would allow to effectively extend the class.
4. Make getBMI use getWeight and getHeight instead.

So what solution you pick is up to you...story time! More times than not I've encountered very "liberate" use of the final keyword and ran into more troubles than it solved. It happens quite often to me that I need to extend a class to either lever out or strap on functionality because my use-case does not perfectly fit the implementation. A final class removes any possibility to do so which means that other ways must be found to do that.

There's also the question what you want to try to achieve. Personally, I got told that using final literately will make the code easier to read and maintain, that is an opinion that I cannot agree with. Because if methods and members are randomly protected, private and private final and made need to be changed every now and then when the logic changes, it does reek more like a cargo-cult like decision than an active design decision.

Enough of the rant, I could talk hours about this and it's slightly outside the scope of this review.

---

import javax.swing.*;
import java.awt.*;

Ideally you'd avoid wildcard imports to make it easier to see what classes are being used (collisions are possible). A good IDE will also automatically manage imports for you.

---

  MainProgram() {

Why is the constructor package-private but the class is public?

---

window = new JFrame("BMI CAlculator");

Typo.

---

  public static void main(String[] args) {
    MainProgram mainProgram;
    mainProgram = new MainProgram();
    mainProgram.show();

  }

Personally I like to keep the main in a class called Main which doesn't contain anything else. It makes it extremely easy to find the entrypoint of the application.

---

results.error("Weight input is not a number");

Include exception message and the value that failed to be parsed in the error message to reduce debugging time.

---

    this.BMI.setText(IMC);
    this.BMI.setForeground(Color.BLACK);
    this.information.setText(information);

Some times you use 'this', some times you don't, be consistent.

---

You're slightly mixing responsibilities here. PersonHealthData is model, service and partly view provider in one. It holds the data, calculates the BMI and provides a user-facing string.

If all you want to do in your application is calculate the BMI, PersonHealthData is completely unnecessary, just do the calculation in EntryPanel and be done with it. There's no need for the class at all.

You could, however, move the calculation to BodyMassIndexCalculator which has the method calculate:

public class BodyMassIndexCalculator {
    public double calculate(double heightInCentimeter, double weightInKilogram);
}

Now we actually would also like to have the class of that value, that would mean that we need some additional information. Let's create an enum that holds the classes.

public enum BodyMassIndexClass {
    VERY_SEVERELY_UNDERWEIGHT,
    SEVERLY_UNDERWEIGHT,
    UNDERWEIGHT,
    ...;
}

That gives us a nice start. Assuming that these BMI classes will never change, we will couple them with their respective values directly:

public enum BodyMassIndexClass {
    VERY_SEVERELY_UNDERWEIGHT(0, 15),
    SEVERLY_UNDERWEIGHT(15, 16),
    UNDERWEIGHT(16, 18.5),
    ...;
    
    private final double from;
    private final double to;
    
    private BodyMassIndexClass(double from, double to) {
        this.from = from;
        this.to = to;
    }

    // TODO Getters for from and to.
    
    public BodyMassIndexClass forIndex(double bodyMassIndexValue) {
        for (BodyMassIndexClass bodyMassIndexClass : values()) {
            if (bodyMassIndexClass.from <= bodyMassIndexValue && bodyMassIndexValue < bodyMassIndexClass.from) {
                return bodyMassIndexClass;
            }
        }
        
        return null;
    }
}

We could pair that with a nice little holder:

public final class BodyMassIndex {
    public final BodyMassIndexClass getBodyMassIndexClass();
    public final double getValue();
}

Note that this is final because it is completely devoid of logic, it merely couples two values together.

That can now be returned from our calculator class.

In the GUI you can now have a switch to get the appropriate text to display.