javascript app code review

Question

Hi I am new to javascript. I would like to get feedback for possible improvements to the code of my little app.

It is a 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 *deal weight range for given height * percentage over/under ideal weight

Thanks for taking the time to look over my code.

Here is the code:

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>

And the formScript.js code :

//***********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;

    }   

}

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
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
Thanks Flambino for your useful comment and information. I have changed my code so that the brace-on-new-line style is removed. — agilesumo, Feb 12 '13 at 9:22

aroth · Answer 1 · 2013-02-12 02:59:28Z

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
};

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");
//...
```
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).

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.

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

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

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.

@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
@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
@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
@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

asked	10 months ago
viewed	285 times
active	10 months ago

Explore our sites

javascript app code review

1 Answer

Your Answer

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

Community Bulletin

Hot Network Questions

Explore our sites

javascript app code review

1 Answer

Your Answer

Sign up or login

Post as a guest

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

Community Bulletin

Related

Hot Network Questions