Body Mass Index Calculator

Question

I created a quick calculator in Javascript and I was wondering if there was a way to make the code look prettier or make it simpler. To me, it looks really cramped and messy and I'm not sure how change that.

function calc() {
    var height = (document.getElementById('height').value) / 100;
    var weight = document.getElementById('weight').value;

    var output = document.getElementById('output');
    var bmi = Math.round((weight / Math.pow(height, 2)) * 10) / 10;

    var result = document.getElementById('result');
    var text = "";
    if (height != "" && weight != "") {
        if (bmi != "NaN" && bmi != 0) {
            if (bmi % 1 != 0) {
                output.innerHTML = bmi;
            } else {
                output.innerHTML = bmi + ".0";
            }
            if (10 <= bmi && bmi < 14) {
                text = "You are seriously underweight!";
            } else if (15 <= bmi && bmi < 18.5) {
                text = "You are slightly underweight.";
            } else if (18.5 <= bmi && bmi < 25) {
                text = "You are healthy!";
            } else if (25 <= bmi && bmi < 30) {
                text = "You are slightly overweight.";
            } else if (30 <= bmi && bmi < 40) {
                text = "You are obese!";
            } else if (40 <= bmi && bmi < 50) {
                text = "You are seriously obese!";
            } else {
                text = "You should be dead...";

            }

            result.innerHTML = text;
        }
    }
}

Answer 1

The reason it seems messy is because you have one function doing multiple things. So what does your function do?

• It queries the DOM for elements containing the input.
• It calculates the BMI based on that input
• It has logic that determines what kind of output to produce
• It produces output to the DOM.

There are a few problems with that:

1. What if you want to run this function in a different DOM? You can't, you're bound to elements with specific IDs.
2. What if you want just the result? You can't, because your function doesn't return anything but explicitly mutates the DOM.
3. It's unclear from the signature what your function needs to work (i.e. available DOM with elements with specific IDs). Functions normally declare what they need in the form of parameters.

So how can we do better? We can divide into several independent functions:

function calculateBMI(height, weight) {
  // todo
  // calculate BMI based on height and weight passed in.
  // returns a number (the BMI)
}

function bmiToHealth(bmi) {
  // todo
  // determine what text to display based on the bmi value.
  // returns a string (the text to display)
}

And use it like this:

function calculateBmiFromDOM() {
    let heightInput = document.getElementById('height');
    let weightInput = document.getElementById('weight');
    let output = document.getElementById('output');
    let result = document.getElementById('result');

    let bmi = calculateBMI(heightInput.value, weightInput.value);
    output.textContent = bmi.toPrecision(3); // 25 -> 25.0, 23.752 -> 23.7
    result.textContent = bmiToHealth(bmi);
}
let form = getFormSomehow(); // usually with getElementById or querySelector
// You can pass functions as parameters in JavaScript
form.addEventListener('submit', calculateBmiFromDOM);

And that last snippet the only thing that's specific to your current DOM, so when your DOM changes, you don't need to change all of your code, just the bit that interacts with it.

Additional notes for future snippets (just general things I've noticed):

• anything != 'NaN' is pointless, nothing in your logic might return a string with the value of 'NaN'. Even if it were NaN, you can't compare it with ==, you need to use the isNaN() function.
• You should always use === or !== when comparing. Using == and != can introduce unexpected behavior due to type conversion.
• Functions work best when they accept values as arguments and/or return values as result.

Answer 2

Another way to do this is to make it data-centric instead of code-centric. In this case, I have introduced an array of objects, each of which has a bmi and msg member. The we simply linearly search through the table looking for a match. Note that this method also avoids the error in the original which has a curious gap between 14 and 15 in which the code would report "You should be dead."

function getBMImsg(bmi) {
  var bmiTable = [{bmi: 10,   msg: "You should be dead"}, 
                  {bmi: 15,   msg: "You are seriously underweight"}, 
                  {bmi: 18.5, msg: "You are slightly underweight"}, 
                  {bmi: 25,   msg: "You are healthy!"}, 
                  {bmi: 30,   msg: "You are slightly overweight"}, 
                  {bmi: 40,   msg: "You are obese!"}, 
                  {bmi: 50,   msg: "You are seriously obese!"}];
  var i;
  var text = bmiTable[0].msg;
  for (i = 0; i < bmiTable.length; i++) {
    if (bmi < bmiTable[i].bmi) {
      text = bmiTable[i].msg;
      break;
    }
  }
  return text;
}

function calcBMI(height, weight) {
  var bmi = 0;
  if (height > 0 && weight > 0) {
    bmi = Math.round((weight / Math.pow(height/100, 2)) * 10) / 10;
  }
  return bmi;
}

function bmiFunction() {
  var height = document.getElementById('height');
  var weight = document.getElementById('weight');
  var output = document.getElementById('output');
  var result = document.getElementById('result');
  var bmi = calcBMI(height.value, weight.value);
  
  if (bmi > 0) {
    output.textContent = bmi.toPrecision(3);
    result.textContent = getBMImsg(bmi)
  }
}
document.getElementById("bmiform").addEventListener("submit", bmiFunction);

<form id="bmiform">
  <p>Height:
    <input id="height" />
  </p>
  <p>Weight:
    <input id="weight" />
  </p>
  <input type="submit" value="calculate" />
</form>
<p id="output" />
<p id="result" />

Answer 3

The reason it looks 'cramped' is because you are putting some of your resources in code.

This section,

        if (10 <= bmi && bmi < 14) {
            text = "You are seriously underweight!";
        } else if (15 <= bmi && bmi < 18.5) {
            text = "You are slightly underweight.";
        } else if (18.5 <= bmi && bmi < 25) {
            text = "You are healthy!";
        } else if (25 <= bmi && bmi < 30) {
            text = "You are slightly overweight.";
        } else if (30 <= bmi && bmi < 40) {
            text = "You are obese!";
        } else if (40 <= bmi && bmi < 50) {
            text = "You are seriously obese!";
        } else {
            text = "You should be dead...";

        }

about the bmi strings, should be changed so that your calculator is not actually responsible for printing the outputs.

If you make 1 variable setting that defines the strings, 1 function that takes height and weight and returns BMI, 1 function that takes a BMI and returns the string to display and 1 function that reads and writes html elements, you've got your code divided into nice organized chunks.

Something like bmiStrings, handleBMICalculationForm, calculateBMI, getBMIString would work.

It would make your main function look like this:

function handleBMICalculationForm() {
    var height = (document.getElementById('height').value) / 100;
    var weight = document.getElementById('weight').value;

    var output = document.getElementById('output');
    var result = document.getElementById('result');

    var bmi = calculateBMI(height, weight);
    if (height != "" && weight != "") {
        if (bmi != "NaN" && bmi != 0) {
            if (bmi % 1 != 0) {
                output.innerHTML = bmi;
            } else {
                output.innerHTML = bmi + ".0";
            }
            var text = getBMIString(bmi);
            result.innerHTML = text;
        }
    }
}

I think there's further room for improvement, mainly by making the validation happen in a separate function.

Answer 4

If this is the only thing you are doing, i.e., if it's not going to be part of some other bigger project with more advanced requirements, then, simple is best. Don't do anything about it! Modularization is better only when there is enough stuff to be done generically and separately. For requirements that are already simple, there is no complexity to hide from the user nor is there a need to introduce in code any more complexity than is found in the requirement itself.

If you intend to do it as a learning experiment in code quality enhancement, then Pimgd's answer is useful. Also, as you learn further to enhance code quality, you would really be moving towards jQuery or other frameworks.