0
\$\begingroup\$

I'm learning JavaScript at the moment (still very much a beginner) and this is the first app I've designed on my own, without using any tutorials. It's an app which takes the user's gender, age, height, and weight and then calculates their BMR. It works, but I would really appreciate it if someone more knowledgeable could take a look at the code to see if it's optimal (or even close!). I want to unlearn any bad habits I may have picked up as soon as possible.

Here's a link to all the code on GitHub, along with a ReadMe that explains in detail how the app works:

https://github.com/neilmurrellcoding/bmr-calculator-version-one

The HTML code is:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">

  <link rel="stylesheet" href="css/index.css">
  <title>BMI Calculator</title>
</head>
<body>
  <div class="first-container">
  <div class="second-container">
    <div class="application">
      <h1 class="title">DIET PLANNER</h1>
      <p class="intro">Fill out the below fields to discover your BMR.</p><br>
      <p class="intro">Your BMR is the number of calories you burn off in 24 hours</p>

      <form>
        <div class="radio-buttons">
          <p class="label"><strong>Select your gender</strong></p>
          <input type="radio" checked="checked" id="male" name="gender" value="male"><label for="male" class="gender-label">Male</label>
          <input type="radio" id="female" name="gender" value="female"><label for="female" class="gender-label">Female</label>
        </div>


        <p class="label"><strong>Age</strong></p>
        <input type="number" id="age" class="age-field" min="0" max="130" placeholder="Enter your age"><br><br><br>
        
        <p class="label"><strong>Height</strong></p>
        <input type="number" id="feet" class="field" min="0" max="9" placeholder="ft">
        <input type="number" id="inches" class="field" min="0" max="11" placeholder="in"><br>

        <p class="label"><strong>Weight</strong></p>

        <input type="number" id="stone" class="field" min="0" max="80" placeholder="stone">
        <input type="number" id="lbs" class="field" min="0" max="13" placeholder="lbs"><br>
        

        <input type="submit" id="submit" class="submit">
      </form>

      
    </div>

  </div>
  <div id="results">
    <div class="results-container">
      <h1 class="title">Your daily BMI is:</h1>
      <p id="bmi-result">Placeholder text</p>
    </div>
  </div>
</div>
  <script src="application.js"></script>
</body>
</html>

The JS code is:

document.getElementById('results').style.display = 'none';

submit.addEventListener('click', function(e) {  // Runs getValues when form's submit button is clicked.
  e.preventDefault();

  getValues();
})

getValues = () => {  // Converts values from form fields into integers, then assigns these integers to variables.
const age = parseInt(document.getElementById("age").value);
const gender = document.getElementsByName("gender"); // This produces a node-list with 2 items, male and female.  Male is checked by default.
const heightFeet = parseInt(document.getElementById("feet").value);
const heightInches = parseInt(document.getElementById("inches").value);
const weightStone = parseInt(document.getElementById("stone").value);
const weightLbs = parseInt(document.getElementById("lbs").value);

x = calculateAge(age); // calls the calculateAge function below, passes 'age' as an argument, assigns return value to x.
y = calculateWeight(weightStone, weightLbs); // calls the calculateWeight function below, passes 'weightStone' & 'weightLbs' as args, assigns return value to y.
z = calculateHeight(heightFeet, heightInches); // calls the calculateHeight function below, passes 'heightFeet' & 'heightWeight' as args, assigns return value to z.
finalResult(x, y, z, gender); // calls finalResult function below, passes return values of above 3 functions as args. 
}


calculateAge = age => { // Multiplies user's age by 5 and saves this as finalAge.
  finalAge = age * 5;
  return finalAge;
}

calculateWeight = (weightStone, weightLbs) => { // Converts user's imperial weight into kg, multiplies it by 10, and returns finalWeight.
  kilogramWeight = ((weightStone * 14) + weightLbs) * 0.453;
  finalWeight = kilogramWeight * 10;
  return finalWeight;
}

calculateHeight = (heightFeet, heightInches) => { // Converts user's imperial height into cm, multiplies it by 6.25, and returns finalHeight.
  centimeterHeight = ((heightFeet * 12) + heightInches) * 2.54;
  finalHeight = centimeterHeight * 6.25;
  return finalHeight;
}

finalResult = (x, y, z, gender) => {
  result = z + y - x; // finalWeight + finalHeight - finalAge.
  for(i = 0; i < gender.length; i++) { // This checks to see which gender the user checked. If 'male', +5 to finalResult. Else, -161 from finalResult.
    if(gender[i].checked) {
      finalResult = result + 5;
      break;
    } else {
      finalResult = result - 161;
      break;
    }
  }
 
  revealResult(finalResult); // Calls below function & passes finalResult as an argument.

  return finalResult;
}

revealResult = finalResult => {
  finalResult = Math.floor(finalResult);
  let finalBmi = finalResult.toString()
  
  document.getElementById('results').style.display = 'block'; // Reveals the 'results' box which was originally hidden (see line 1)
  const calories = document.getElementById("bmi-result"); // Grabs 'p' element.
  calories.classList.add("finalNumberStyling") // Applies simple styling to 'p' element.
  calories.innerText = finalBmi + ' calories per day'; // Inserts finalBmi string into 'p' element.
}

I've not included the CSS code because the styling is very minimal, but it's all available at the GitHub link just in case.

Any comments or suggestions you have would be very much appreciated.

Thanks.

EDIT: Made a few edits to the JS code to add some explanatory notes.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Poor naming

The first thing that popped out at me was the naming. Starting with BMI

(BMI) Body mass index is not a time dependent quantity. You show a calculated value under the heading <h1 class="title">Your daily BMI is:</h1> which makes no sense at all.

I don't know what the value you are calculating is. I will assume that the string '" calories per day"' found in the function revealResult defines the value you calculate.

Some more naming problems

  • getValues What values? Maybe use calculateDailyCalories

  • 3 functions prefixed with calculate? but the user gives you the age weight and height. You are not calculating these values you are calculating some metric from the age weight and height.

    • calculateAge can be ageMetric
    • calculateWeight can be weightMetric
    • calculateHeight can be heightMetric
  • finalResult Great its final, but what is it? Maybe dailyCalories or just calories would be better

  • The arguments for finalResult(x, y, z, gender) ? This is very bad as it give no indication as to what they represent. Maybe use (ageMetric, weightMetric, heightMetric, isMale)

  • revealResult A little odd to use reveal, but reveal result give no indication as to what you are revealing. Maybe use displayDailyCalories or displayCalories

Keep variable names as short as possible. For example the function calculateHeight(heightFeet, heightInches) It can be inferred that the variables heightFeet heightInches represent heights, thus the better names are feet and inches

Element names (ids) also do not give any indication as to what they are. See rewrite HTML

Strict Mode

To help spot bad habits early always use Strict_mode. This is done by adding the directive "use strict" as the first line of the JS file or script tag

ALWAYS declare variables

If you used strict mode your code would have thrown errors for all the undeclared variables. Always declare variables using const, var or let.

Comments

If you feel you need to add comments to explain the code, then first consider improving the naming and layout of the code. when commenting the assumption is that the reader knows how to code. If they don't know how to code they should not be editing your code in the first place.

Further points

  • Maintain a consistent style. In some places you neglected to add semi colon to the end of lines.

  • Use functions to reduce the amount of repeated code. (see rewrite)

  • Avoid single use variables.

  • Use textContent rather than innerText to set an elements text.

  • The code that check for gender is a very obscure. Make sure that the code is not ambiguous and that you do not need to hunt around to find out what the code does.

CSS

Use a style class rather than directly setting the style property. For example hiding and showing the results you have

document.getElementById('results').style.display = 'none';
// then later in a function
   document.getElementById('results').style.display = 'block'; 

Rather create a style rule. Add the class to the HTML as default. Then remove the rule when you need to show the content

/* CSS */
.hidden {display: none}

<!-- HTML -->     
<div id="results" class="hidden">

// JavaScript
document.getElementById('results').classList.remove("hidden")

Don't add code when not needed

Why do you add the class calEl.classList.add("finalNumberStyling"); in code. The element is hidden and you only add this when you show the element. Rather It should already be in the HTML

Rewrite

The rewrite adds some helper functions to remove the verbose DOM calls. byId, byName gets elements by id and name. valById gets a value of an input by its id

The rest of the rewrite removes a lot of the code noise. Single use variables, Verbose DOM calls.

The gender is taken directly from the maleRadio button (not iterating over the gender radio buttons) and passed to calories if isMale is false then not male.

Rather than listen for the submit buttons click event the rewrite listens for the forms submit event. To add the listener the form has been given the id 'caloriesForm'

I replaced some of the input headings with label tags. All the inputs should use label tags to label them rather than just text elements near them.

"use strict";
const byId = id => document.getElementById(id);
const byName = name => document.getElementsByName(name);
const valById = id => parseInt(byId(id).value, 10);

const ageMetric = age => age * 5;
const weightMetric = (stone, lbs) => (stone * 14 + lbs) * 0.453 * 10;
const heightMetric = (feet, inches) => (feet * 12 + inches) * 2.54 * 6.25;

const calories = (ageMetric, weightMetric, heightMetric, isMale) => 
    Math.floor(heightMetric + weightMetric - ageMetric + (isMale ? 5 : -161)); 
    
const displayCalories = calories => {
    byId("resultsContainer").classList.remove("hidden");
    byId("dailyCaloriesDisplay").textContent = calories + ' calories per day'; 
}

byId("caloriesForm").addEventListener('submit', event => {  
    event.preventDefault();
    displayCalories(
        calories(
            ageMetric(valById("ageInput")), 
            weightMetric(valById("stoneInput"), valById("lbsInput")), 
            heightMetric(valById("feetInput"), valById("inchesInput")),
            byId("maleRadio").checked
        )
    );
})
.hidden {display: none}
<form id="caloriesForm">
    <div class="radio-buttons">
        <label for="gender" class="label"><strong>Gender</strong></label >
        <input type="radio" checked="checked" id="maleRadio" name="gender"><label for="male" class="gender-label">Male</label>
        <input type="radio" name="gender"><label for="female" class="gender-label">Female</label>
    </div>

    <label for="ageInput" class="label"><strong>Age</strong></label >
    <input type="number" id="ageInput" class="age-field" min="0" max="130" placeholder="Enter your age"><br>

    <p>Height</p>
    <label for="feetInput" class="label"><strong>Feet</strong></label >
    <input type="number" id="feetInput" class="field" min="0" max="9" placeholder="ft">
    <label for="inchesInput" class="label"><strong>inches</strong></label >
    <input type="number" id="inchesInput" class="field" min="0" max="11"  value="0"><br>

    <p>Weight</p>
    <label for="stoneInput" class="label"><strong>Stone</strong></label >
    <input type="number" id="stoneInput" class="field" min="1" max="80" placeholder="stone">
    <label for="lbsInput" class="label"><strong>lbs</strong></label >
    <input type="number" id="lbsInput" class="field" min="0" max="13" value="0"><br>

    <input type="submit" class="submit">
</form>

<div id="resultsContainer" class="hidden" class="results-container hidden">
    <p id="dailyCaloriesDisplay" class="finalNumberStyling"></p>
</div>

More to do.

You code does not check if the form has been completed. Displaying the value NaN (not a number) if any of the fields have not been set.

You should detect if there are missing fields and prompt the use to add the missing information.

Only when all fields are entered should you display the result.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks so much for the detailed feedback. That was exactly the kind of thing I was after. \$\endgroup\$
    – Nellington
    Commented Feb 8, 2021 at 18:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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