Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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;
        }
    }
}
share|improve this question
    
BMI is also based on age, which is not a factor here. – Quill Jan 11 at 9:44
    
@Quill That's debatable, usually it's just height and weight. – Mast Jan 11 at 14:26
4  
One thing to be careful with in JS is the difference between a string and a number. You can treat strings like numbers, but you should always explicitly convert them with parseInt or parseFloat. What if someone entered Tall as their height? "Tall" / 100 is NaN, which you certainly don't want. – Bailey Parker Jan 11 at 17:26
up vote 20 down vote accepted

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.
share|improve this answer
    
It might be worth pointing out that let is an ES6 construct (and a very nice one at that). Alas, this means that it has incomplete (but still good) support on current JS engines. – senshin Jan 12 at 6:43
1  
Yes, I like to sprinkle those to help people learn about it (and promote it at the same time). In this particular case, let is a drop-in replacement for var. – Madara Uchiha Jan 12 at 8:06

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" />

share|improve this answer
    
You have several bad practices which are unaddressed (like equating with string NaN (which doesn't make sense), the use of == in place of === and the inline event handler. – Madara Uchiha Jan 11 at 13:26
    
I don't understand what you mean when you say "the inline event handler." – Edward Jan 11 at 13:34
    
form onsubmit="calc();" as opposed to using addEventListener() or something similar. – Madara Uchiha Jan 11 at 13:34
    
Seriously? For a ten line snippet? – Edward Jan 11 at 13:40
1  
Absolutely. Also see my first comment on Whirl Mind's answer. – Madara Uchiha Jan 11 at 13:41

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.

share|improve this answer
1  
I'd also add that you don't need to put both a greater than and less than comparison in each of the else if statements - only the upper bound of a region is needed, since it will 'flow' down from higher statements – costrom Jan 12 at 0:31

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.

share|improve this answer
8  
From past experience, "it's not going to be part of some other bigger project" very rapidly turns to "This is the production code our company has been running for 15 years now." – Madara Uchiha Jan 11 at 12:19
1  
That's been my experience too @MadaraUchiha, but Whirl Mind makes a good point. If there was another, more thorough answer here, I might upvote it, but that would mark it as "answered" when searching and I don't wish that to happen because this code could be simplified. – RubberDuck Jan 11 at 12:50
    
@MadaraUchiha : Very true. That's the time to look at branching code out into multiple functions. – Whirl Mind Jan 11 at 13:03

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.