Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is an HTML / JavaScript app for calculating someones body mass index. The app does conversion of metric to standard measurements and standard to metric.

It outputs:

  • BMI
  • weight category
  • ideal weight range for given height
  • percentage over/under ideal weight

index.html

<?xml version="1.0" encoding="UTF-8" ?>
<!DOCTYPE html PUBLIC
  "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">

<head>
    <title>BMI Calculator</title>
    <style type="text/css">
        #mainDisplay
        {
            padding:25px;
        }
        #form
        {
            width:250px;
        }
        #results
        {
            background-color:#E6E6E6;
            width:450px;                    
        }     
    </style>
</head>

<body>
    <!-- ... action="PrintBMI.jsp" method="post" onsubmit="calculateBMI()" ... left out for now from <form> below -->
    <div id = "mainDisplay">
        <form name ="bmiForm" id = "form" action = "index.html">
            <fieldset>
                <legend>BMI Calculator</legend>
                <h3>Enter your weight</h3>
                Stone <input type = "text" name = "stone" size = "1" maxlength = "2" />
                Pounds <input type = "text" name = "pounds" size = "2" maxlength = "6" />
                <br />
                <strong>OR</strong>
                <br />
                KGs <input type = "text" name = "kgs" size = "2" maxlength = "6" />

                <h3>Enter your height</h3>
                Feet <input type = "text" name = "feet" size = "1" maxlength = "1" />
                Inches <input type = "text" name = "inches" size = "1" maxlength = "4" />
                <br />
                <strong>OR</strong>
                <br />
                CMs <input type = "text" name = "cms" size = "2" maxlength = "6" />
                <p></p><input id = "button" type="button" value = "Find out your BMI" />
            </fieldset>
        </form>
    </div>

    <script type="text/javascript" src = "js/formScript.js"></script>

</body>
</html>

formScript.js

//***********Constants***********

var POUNDS_IN_STONE = 14;
var KGS_PER_POUND = 0.453592;
var INCHES_IN_FOOT = 12;
var CMS_PER_INCH = 2.54;
var CMS_PER_METRE = 100;
var INPUT_ERROR_MSG = "That was not a valid measurement unit. Please try again";
var IDEAL_BMI_LOWER = 18.5;
var IDEAL_BMI_UPPER = 25;

//***********Global Variables***********

var stoneField = document.forms["bmiForm"]["stone"];
var poundsField = document.forms["bmiForm"]["pounds"]
var kgsField = document.forms["bmiForm"]["kgs"];
var feetField = document.forms["bmiForm"]["feet"];
var inchesField = document.forms["bmiForm"]["inches"];
var cmsField = document.forms["bmiForm"]["cms"];

//***********Register Event handlers***********

document.getElementById("button").onclick = function() { outputBmi() };
stoneField.onchange = function() { updateForm( "stonefld" ) };
stoneField.onchange = function() { updateForm( "poundsfld" ) };
kgsField.onchange = function() { updateForm( "kgsfld" ) };
feetField.onchange = function() { updateForm ( "feetfld" ) };
inchesField.onchange = function() { updateForm ( "inchesfld" ) };
cmsField.onchange = function() { updateForm ( "cmsfld" ) };

//***********Helper rounding number Functions***********

function roundTwoDecimals ( number )
{
    return  Math.round ( number * 100 )  / 100 ; 
}

function roundOneDecimal ( number )
{
    return  Math.round ( number * 10 )  / 10 ; 
}

function toInteger(number)
{ 
    return Math.round ( Number ( number ) ); 
}


function toIntegerFloor ( number )
{
    return Math.floor ( Number ( number) ); 
}

//***********Helper function for updateForm***********


// deal with a Not a Number input in the specified field
function processNaNInput ( field )
{
    field.value = "";
    field.focus();
    alert(INPUT_ERROR_MSG);
}

//**********************

//  updates the fields for the corresponding units of measure after a field is updated
function updateForm ( updatedField )
{
    if ( updatedField === "stonefld" || updatedField === "poundsfld" )
    {   
        if ( isNaN ( stoneField.value ) )
        {
            processNaNInput (stoneField);
        }
        else if ( isNaN ( poundsField.value ) )
        {
            processNaNInput(poundsField);

        }
        else if ( poundsField.value === "" && stoneField.value === "" )
        {
            kgsField.value = "";
        }   
        else
        {
            var stoneFldInKgs = ( stoneField.value * POUNDS_IN_STONE ) * KGS_PER_POUND ;
            var poundsFldInKgs = poundsField.value * KGS_PER_POUND;
            var totalKgs = stoneFldInKgs + poundsFldInKgs;
            kgsField.value = roundTwoDecimals ( totalKgs );
        }
    }

    else if ( updatedField === "kgsfld" )
    {
        if ( isNaN ( kgsField.value ) )
        {
            processNaNInput( kgsField );        
        }   
        else if ( kgsField.value === "")
        {
            stoneField.value = "";
            poundsField.value = "";
        }   
        else
        {   
            var totalKgsInPounds = kgsField.value / KGS_PER_POUND;
            var numStone = toIntegerFloor ( totalKgsInPounds / POUNDS_IN_STONE );
            var numPounds = roundTwoDecimals ( totalKgsInPounds % POUNDS_IN_STONE );

            stoneField.value =  numStone;
            poundsField.value = numPounds;
        }   
    }

    else if ( updatedField === "feetfld" || updatedField === "inchesfld" )
    {   
        if ( isNaN ( feetField.value ) )
        {
            processNaNInput(feetField);     

        }   
        else if ( isNaN ( inchesField.value ) )
        {
            processNaNInput(inchesField);       

        }   
        else if ( feetField.value === "" && inchesField.value === "" )
        {
            cmsField.value = "";
        }
        else
        {
            var feetFldInCms = ( feetField.value * INCHES_IN_FOOT ) * CMS_PER_INCH;
            var inchesFldInCms = inchesField.value * CMS_PER_INCH;
            var totalCms = feetFldInCms + inchesFldInCms;
            cmsField.value = roundTwoDecimals ( totalCms ); 
        }
    }

    else if ( updatedField === "cmsfld" )
    {
        if ( isNaN ( cmsField.value ) )
        {
            processNaNInput(cmsField);      

        }    
        else if ( cmsField.value === "" )
        {
            feetField.value = "";
            inchesField.value = "";
        }
        else
        {
            var cmsFldInInches = cmsField.value / CMS_PER_INCH;
            var numFeet = cmsFldInInches / INCHES_IN_FOOT;
            var numInches = cmsFldInInches % INCHES_IN_FOOT;
            feetField.value = toIntegerFloor ( numFeet );
            inchesField.value = roundOneDecimal ( numInches );
        }
    }

    else
    {
        alert ( "Invalid parmater passed to completeForm function" )
    }

}   

//***********Helper Functions Called by calculateBMI***********


// check if any of the fields in the form are empty and if empty outputs
// an error message. Returns false is the form is fully filled out 
function hasEmptyFlds ( )
{

        // If all fields are empty output a message
    if ( ( stoneField.value === null || stoneField.value === "" ) && ( poundsField.value === null || poundsField.value === "" ) && ( kgsField.value === null || kgsField.value === "" ) &&
        ( feetField.value === null || feetField.value === "" ) && ( inchesField.value === null || inchesField.value === "" ) && ( cmsField.value === null || cmsField.value === "" ) )
    {
      alert( "Please enter a weight and height" ); 

      return true;
    }
    // If weight fields of the form are not filled out output an message
    else if (  kgsField.value === null || kgsField.value === "" ) 
    {
        alert ( "Please enter a weight" );
        return true;
    } 
    // If the height fields of the form are not filled out output a message
    else if (  cmsField.value === null || cmsField.value === ""  )
    {
        alert ( "Please enter a height" );
        return true;
    }
    else
    {
        return false;
    }   
}

// returns the name of the bmi category for a given bmi
function getBmiCategory ( bmi )
{
    if ( bmi < 15 )
    {
        return "Very severely underweight";
    }
    else if ( bmi < 16 )
    {
        return "Severely underweight";
    }   
    else if ( bmi < 18.5 )
    {
        return "Underweight";
    }
    else if ( bmi < 25 )
    {   
        return "Normal (healthy weight)";
    }   
    else if ( bmi < 30 )
    {
        return "Overweight";
    }   
    else if ( bmi < 35 )
    {
        return "Moderately obese";
    }   
    else if ( bmi < 40 )
    {
        return "Severely obese";
    }
    else
    {
        return "Very severely obese";
    }   
}

//**********************

// calculates and outputs bmi, bmi category, ideal bmi range, ideal weight range and percent over or under ideal weight 
function outputBmi ( )
{
    if ( ! hasEmptyFlds ( )  )
    {
        var heightInMetres = cmsField.value / 100;
        var bmi = roundOneDecimal ( kgsField.value /  ( heightInMetres * heightInMetres ) );
        var category = getBmiCategory ( bmi ); 

        //lower weight cut off for a healthy bmi
        var lowerKgs = toInteger ( IDEAL_BMI_LOWER * ( heightInMetres * heightInMetres ) );
        //upper weight cut off for a healthy bmi
        var upperKgs = toInteger ( IDEAL_BMI_UPPER * ( heightInMetres * heightInMetres ) );         

        //convert lower cut off from kgs to stone and pounds
        var totalLowerPounds = lowerKgs / KGS_PER_POUND;
        var lowerStone = toIntegerFloor  ( totalLowerPounds / POUNDS_IN_STONE );
        var lowerPounds = toInteger ( totalLowerPounds % POUNDS_IN_STONE );

        //convert upper cut off from kgs to stone and pounds
        var totalUpperPounds = upperKgs / KGS_PER_POUND;
        var upperStone = toIntegerFloor  ( totalUpperPounds / POUNDS_IN_STONE );
        var upperPounds = toInteger ( totalUpperPounds % POUNDS_IN_STONE );     

        var html = "<p id = 'results'>";
        html += "Your BMI is : " + bmi + " <br /> Your BMI is in the " + category + " category<br />";

        html += "<br />The BMI for Normal ( healthy weight ) is between 18.5 and 25";
        html += "<br /><br />Normal (healthy weight) for your height is between:<br />";
        html += lowerKgs + " kgs (" + lowerStone + " stone, " + lowerPounds + " pounds ) and ";
        html += upperKgs + " kgs (" + upperStone + " stone, " + upperPounds + " pounds ) <br />";

        if ( kgsField.value < lowerKgs )
        {
            var kgsUnderIdeal = lowerKgs - kgsField.value;
            var percentUnder = toInteger ( ( kgsUnderIdeal * 100 ) / lowerKgs );
            html += "<br />You are " + percentUnder + "% below your ideal weight bracket";                      
        }   
        else if ( kgsField.value > upperKgs )
        {
            var kgsOverIdeal = kgsField.value - upperKgs;
            var percentOver = toInteger ( ( kgsOverIdeal * 100 ) / upperKgs );
            html += "<br />You are " + percentOver + "% over your ideal weight bracket";
        }

        html += "<br /><br /><a href='/'>< Go back to bmi form</a></p>"         

        document.getElementById( 'mainDisplay' ).innerHTML = html;

    }   

}
share|improve this question
    
I know sometimes things are purely up to a dev in the way they style their code, but please, try not to do this: type = "text" and instead remove the spaces on either side of the equal sign type="text". –  jsanc623 Feb 11 '13 at 16:30
4  
Spaces are fine - I use the same style myself - and no-spaces is fine too; use what you prefer. But best to avoid the brace-on-new-line style when writing JavaScript. JS will sometimes try to insert semi-colons in your code if it finds an end-of-line, meaning that linebreaks can break your code. Spaces, however, can't. So never mind jsanc623's comment about that. –  Flambino Feb 11 '13 at 17:25
2  
I think @jsanc623's comment was about the HTML attributes--the one place where I omit spaces around an equals sign "operator". Either way, be consistent: name ="bmiForm" is not cool. –  David Harkness Apr 30 at 20:45
    
That is correct @DavidHarkness - I meant the HTML attributes (i.imgur.com/roguC4C.png) –  jsanc623 May 1 at 2:37
add comment

2 Answers 2

  1. Constants; instead of creating a bunch of standalone keys I'd group them in a single object, like:

    var bmiConstants = {
        POUNDS_IN_STONE: 14,
        KGS_PER_POUND: 0.453592,
        INCHES_IN_FOOT: 12,
        CMS_PER_INCH: 2.54,
        CMS_PER_METRE: 100,
        INPUT_ERROR_MSG: "That was not a valid measurement unit. Please try again",
        IDEAL_BMI_LOWER: 18.5,
        IDEAL_BMI_UPPER: 25
    };
    
  2. Globals; I would prefer using ids on the input fields (or even better, a jQuery selector) to retrieve them rather than going through document.forms. Like:

    var stoneField = document.getElementById("stones");
    var poundsField = document.getElementById("pounds");
    //...
    
  3. Globals; Even more than above, I'd prefer not having these globals at all. Instead you could retrieve them as needed in your functions (and/or pass them in as parameters).

  4. Event Handlers; If you're just going to use the onclick and onchange attributes, there's no need to bind them programmatically. You can set them up in your markup instead, like:

    <input type = "text" name = "stone" size = "1" maxlength = "2" onchange="updateForm(this);" />
    <input type = "text" name = "pounds" size = "2" maxlength = "6" onchange="updateForm(this);" />
    <!-- ... -->
    
    <input id = "button" type="button" value = "Find out your BMI" onclick="outputBmi();" />
    

    Note that this allows you to easily pass the input field itself to updateForm(), instead of just an arbitrary key.

  5. Functions; I'd recommend the following style when writing functions:

    function roundTwoDecimals(number) {
        return Math.round(number * 100) / 100; 
    }
    
  6. Separation of logic and presentation; Instead of composing a big long HTML string in outputBmi() I'd recommend having the basic structure of the output as part of your markup. For instance, contained within a div that is set to display: none initially. Then the only thing outputBmi() needs to do is update the parts that actually change and toggle the visibility of the div. That will result in less code and be more maintainable than trying to construct the entire HTML snippet programmatically.

share|improve this answer
2  
Not sure why you got -1, I disagree wit (4) though. –  konijn Feb 12 '13 at 20:46
2  
@agilesumo - #1) In JavaScript, variables declared without an enclosing context wind up in the global scope by default. Generally speaking, it is considered good practice to minimize the number of things that are declared globally when writing JavaScript. The modified version has one global variable instead of the original 8. There are also slightly more complex techniques that can be used to avoid have any globals at all. –  aroth Feb 16 '13 at 7:45
2  
@agilesumo - #2) This is partly personal preference, but my feeling is that using document.forms is a more brittle approach. You're relying on two things there, the name of the form, and the name of the input field/parameter. If another developer comes along and changes one of those, your code will break in a non-obvious way. With getElementById you're relying on only a single piece of information to find your element(s), which makes it less brittle in general, and easier to track down issues if the id ever does get changed. –  aroth Feb 16 '13 at 7:58
1  
@agilesumo - #3) Is partly related to #1. Usually in JavaScript you want to avoid using global variables if you can help it. And if you're using a framework like jQuery, looking up an element on demand doesn't take must more code than $("#kgsField").val()...which is only a few characters longer than kgsField.value. Even without a framework there are ways to avoid having the globals, such as having a getFields() function that looks up and returns all the fields in a single object. –  aroth Feb 16 '13 at 8:07
1  
@agilesumo - And the other part of #3 is that by looking up the element once and caching the result forever in the global variable, you're pretty much excluding the possibility of doing anything dynamic with the section of the page that contains those fields (i.e. anything that would change the innerHTML of the element that contains them). Even if you don't plan on doing anything like that in this case, I think it's generally better to code in a way that at least allows for the possibility in the future. –  aroth Feb 16 '13 at 8:13
show 5 more comments

The Markup

<?xml version="1.0" encoding="UTF-8" ?>
<!DOCTYPE html PUBLIC
  "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">

Any particular reason for using XHTML? The typical way to go these days is HTML5 – also makes the doctype easier. And it will have all the Web 2.0 features.

<style type="text/css">
    #mainDisplay
    {
        padding:25px;
    }
    #form
    {
        width:250px;
    }
    #results
    {
        background-color:#E6E6E6;
        width:450px;                    
    }     
</style>

Uh-oh! Don't do CSS (or Javascript for that matter) inline. You are violating the Separation of Concerns pattern. As a general rule: Any file should only ever contain two languages at most – the language the program is written in and English for documentation, if necessary.

The Javascript

First off: Don't clutter the global space. Never – just never. It's so easy to at least use a closure to hide your stuff!

Next up, don't use document.forms, just get the element by id. You can also avoid unnecessary DOM lookups by caching the the DOM element.

document.getElementById("button").onclick = function() { outputBmi() };

There is no reason to wrap anything. Just do

document.getElementById("button").onclick = outputBmi;

Now to this:

function roundTwoDecimals ( number )
{
    return  Math.round ( number * 100 )  / 100 ; 
}

function roundOneDecimal ( number )
{
    return  Math.round ( number * 10 )  / 10 ; 
}

Instead of duplicating methods that only differ slightly, abstract it and create a single method that simply takes a parameter to specify how many digits the number should be rounded to.

alert(INPUT_ERROR_MSG);

Don't use alert. Ever. It has terrible user experience and most browsers tempt the user to deactivate Javascript for the entire page. Use DOM methods to display appropriate messages instead.

function updateForm ( updatedField )
    // … and the entire thing that follows

Warning: Monster method detected. You are using a single parameter with a known set of values to decide what the entire function does – this is a strong sign that you should instead declare individual functions without parameters.

Your function is also way too long. A good function does one thing and it does it well. A good function is no longer than a few lines of code. Your function does too much and it is too hard to keep track of what it is doing.

function hasEmptyFlds ( )

Do you like shrtng wrds? Don't do that – don't be lazy.

function getBmiCategory ( bmi )
    // and the entire rest

This method clearly shows the too tight coupling of data and logic. A better way to write such methods is to declare an object that simply holds thresholds and the messages and a method that uses this object and programmatically chooses the correct message from it. Also makes it easier to understand and maintain.

A few more things:

  • If you're going to document functions, use proper JSDoc, not single comments.
  • Don't construct a big string of HTML, use DOM methods instead. The browser does all of this for you, so let it do its work!
  • You need to work on a lot of the names. updateForm is generic. What does it really tell the caller? Update which form? Update what?

Frankly, there is a lot more to say, but I feel that this is enough to deal with first.

share|improve this answer
add comment

Your Answer

 
discard

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