2
\$\begingroup\$

As I am still new to web development, refactoring my code is still something that gives me some trouble. I would really appreciate it if someone could look over the Javascript for an interest rate calculator I built, and maybe give me some pointers on how to refactor it/improve on its efficiency.

It consists of three functions:

The first, createOptions, just loops over the numbers 1 to 100, and appends each of these numbers as an option to the drop down menu for the percentage input.

The second, calculateInterest, is self-explanatory. The formula used assumes compound interest. It also contains a loop which iterates over each payment until the payment period entered is reached. By compound interest, I mean the percentage of the initial amount (the principal) representing the interest rate is calculated, added to the principal, and the percentage of their sum (the interest plus the principal) is added to that. This process repeats for each iteration of the loop.

The last function onClick gets the form data in the DOM, stores them in variables, then invokes calculateInterest, passing the variables into its parameters.

I am mainly interested in advice regarding my Javascript but any HTML and CSS pointers would be appreciated, too.

createOptions();

function createOptions(){
  let select = document.getElementById('percentage');
  for(var i = 0; i <= 100; i++){ 
  var node = document.createElement("option");
  node.innerHTML = i;
  node.setAttribute('value', i);
  node.setAttribute('class', 'percent');
  select.appendChild(node); 
  }
}


function calculateInterest(amount, payments, interest){
  var total = amount;
  for(var i = 1; i <= payments; i++){
    var percent = total * interest;
    total = total += percent;
  }
  return '$' + total.toFixed(2); 
}

function onClick(){

  var para = document.getElementById('show');
  var result = document.getElementsByTagName('div')

  var select = document.getElementById('percentage');
  var percentValue = select.options[select.selectedIndex].value / 100;
  var amounts = document.getElementById('amount');
  var amountValue = parseFloat(amounts.value);
  var time = document.getElementById('time');
  var timeValue = parseInt(time.value);

  if(para.className === "show test"){
    para.remove()
    var para = document.createElement('p');
    para.id = "show"
    var result = document.getElementById('result')
    result.appendChild(para);
  }

  para.innerHTML = calculateInterest(amountValue, timeValue, percentValue);
  para.className = "show";
  para.className += " test"

}
body {
  background: linear-gradient( rgb(25, 25, 230), rgb(255, 255, 255));
  background-repeat: no-repeat;
  box-sizing: border-box;

    }

    h1 {
      text-align: center;

    }

    #title {
      font-family: Verdana;
      text-transform: uppercase;
      color: white;
    }
    label {
      font-weight: bolder;
      color: black;
    }
    .btn, .btn-light {
      margin-top: 10px;

    }

    #result {
      margin-top: 10px;
      font-size: 24px;
      text-align: center;

    }

    .wrap {
      position: relative;
      left: 45%;
    }

    .show {
      animation: fadeIn 1.8s ease-in 0.2s 1 normal both running;
    }

    @keyframes fadeIn {
      from {
        opacity: 0; 
      }

      to {
        display: block;
        opacity: 1;
      }
    }

    .test {

      font-size: 60px;
    }
<link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.1.3/css/bootstrap.min.css" integrity="sha384-MCw98/SFnGE8fJT3GXwEOngsV7Zt27NXFoaoApmYm81iuXoPkFOJwJ8ERdknLPMO" crossorigin="anonymous">
<link rel="stylesheet" type="text/css" href="./style.css">


  <h1 id = "title">Interest Rate Calculator</h1>
<div class="container">
 <div class = "wrap">
   <label>% INTEREST</label>
   <select class = 'form-control' style = "width: 10%" name="percentage" id= "percentage"></select>

   <label> $ AMOUNT</label>
   <input type="number" min = '1' max = '999999999' name = "amount" id = "amount" class = "form-control" style = "width: 10%">
   <label for=""># OF PAYMENTS</label>
   <input type = "number" min = '1' max = '999' id = "time" class = "form-control" style = "width: 10%">
    <button onClick = "onClick()" target = "_top" class = 'btn btn-light'>CALCULATE!</button>  
   </div>
 </div>


 <div id="result">
   <p id = "show"></p>
 </div>
  <script type="text/javascript" src = "./script.js"></script>

\$\endgroup\$
1
\$\begingroup\$

Style

You should consistently indent your CSS. This yields the following:

body {
    background: linear-gradient(#1919e6 ,#fff);
    background-repeat: no-repeat;
    box-sizing: border-box
}

h1 {
    text-align: center
}

#title {
    font-family: Verdana;
    text-transform: uppercase;
    color: #fff
}

label {
    font-weight: bolder;
    color: #000
}

.btn,.btn-light {
    margin-top: 10px
}

#result {
    margin-top: 10px;
    font-size: 24px;
    text-align: center
}

.wrap {
    position: relative;
    left: 45%
}

.show {
    animation: fadeIn 1.8s ease-in .2s 1 normal both running
}

@keyframes fadeIn {
    0% {
        opacity: 0
    }

    to {
        display: block;
        opacity: 1
    }
}

.test {
    font-size: 60px
}

Naming

You should name the onClick function something more descriptive. In addition, the for loop should be indented.

createOptions();

function createOptions(){
  let select = document.getElementById('percentage');
  for(var i = 0; i <= 100; i++){
     var node = document.createElement("option");
     node.innerHTML = i;
     node.setAttribute('value', i);
     node.setAttribute('class', 'percent');
     select.appendChild(node);
  }
}


function calculateInterest(amount, payments, interest){
  var total = amount;
  for(var i = 1; i <= payments; i++){
    var percent = total * interest;
    total = total += percent;
  }
  return '$' + total.toFixed(2);
}

function handler(){

  var para = document.getElementById('show'),
      result = document.getElementsByTagName('div')

  var select = document.getElementById('percentage'),
      percentValue = select.options[select.selectedIndex].value / 100,
      amounts = document.getElementById('amount'),
      amountValue = parseFloat(amounts.value),
      time = document.getElementById('time'),
      timeValue = parseInt(time.value);

  if(para.className === "show test"){
    para.remove();
    para = document.createElement('p');
    para.id = "show";
    result = document.getElementById('result');
    result.appendChild(para);
  }

  para.innerHTML = calculateInterest(amountValue, timeValue, percentValue);
  para.className = "show";
  para.className += " test";
}

HTML

Indentation could be a little better, and a doctype should be declared:

<!DOCTYPE html>
<html>

<head>
    <link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.1.3/css/bootstrap.min.css" integrity="sha384-MCw98/SFnGE8fJT3GXwEOngsV7Zt27NXFoaoApmYm81iuXoPkFOJwJ8ERdknLPMO" crossorigin="anonymous">
    <link rel="stylesheet" type="text/css" href="./style.css">
</head>

<body>
    <h1 id="title">Interest Rate Calculator</h1>
    <div class="container">
        <div class="wrap">
            <label>% INTEREST</label>
            <select class="form-control" style="width: 10%" name="percentage" id="percentage"></select>

            <label> $ AMOUNT</label>
            <input type="number" min="1" max="999999999" name="amount" id="amount" class="form-control" style="width: 10%">
            <label for=""># OF PAYMENTS</label>
            <input type="number" min="1" max="999" id="time" class="form-control" style="width: 10%">
            <button onclick="onClick()" target="_top" class="btn btn-light">CALCULATE!</button>
        </div>
    </div>

    <div id="result">
        <p id="show"></p>
    </div>
    <script type="text/javascript" src="./script.js"></script>
</body>

</html>

Rewrite:

createOptions();

function createOptions(){
  let select = document.getElementById('percentage');
  for(var i = 0; i <= 100; i++){
     var node = document.createElement("option");
     node.innerHTML = i;
     node.setAttribute('value', i);
     node.setAttribute('class', 'percent');
     select.appendChild(node);
  }
}


function calculateInterest(amount, payments, interest){
  var total = amount;
  for(var i = 1; i <= payments; i++){
    var percent = total * interest;
    total += percent;
  }
  return '$' + total.toFixed(2);
}

function handler(){

  var para = document.getElementById('show'),
      result = document.getElementsByTagName('div')

  var select = document.getElementById('percentage'),
      percentValue = select.options[select.selectedIndex].value / 100,
      amounts = document.getElementById('amount'),
      amountValue = parseFloat(amounts.value),
      time = document.getElementById('time'),
      timeValue = parseInt(time.value);

  if(para.className === "show test"){
    para.remove()
    para = document.createElement('p');
    para.id = "show"
    result = document.getElementById('result')
    result.appendChild(para);
  }

  para.innerHTML = calculateInterest(amountValue, timeValue, percentValue);
  para.className = "show";
  para.className += " test"

}
body {
    background: linear-gradient(#1919e6 ,#fff);
    background-repeat: no-repeat;
    box-sizing: border-box
}

h1 {
    text-align: center
}

#title {
    font-family: Verdana;
    text-transform: uppercase;
    color: #fff
}

label {
    font-weight: bolder;
    color: #000
}

.btn,.btn-light {
    margin-top: 10px
}

#result {
    margin-top: 10px;
    font-size: 24px;
    text-align: center
}

.wrap {
    position: relative;
    left: 45%
}

.show {
    animation: fadeIn 1.8s ease-in .2s 1 normal both running
}

@keyframes fadeIn {
    0% {
        opacity: 0
    }

    to {
        display: block;
        opacity: 1
    }
}

.test {
    font-size: 60px
}
<!DOCTYPE html>
<html>

<head>
    <link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.1.3/css/bootstrap.min.css" integrity="sha384-MCw98/SFnGE8fJT3GXwEOngsV7Zt27NXFoaoApmYm81iuXoPkFOJwJ8ERdknLPMO" crossorigin="anonymous">
    <link rel="stylesheet" type="text/css" href="./style.css">
</head>

<body>
    <h1 id="title">Interest Rate Calculator</h1>
    <div class="container">
        <div class="wrap">
            <label>% INTEREST</label>
            <select class="form-control" style="width: 10%" name="percentage" id="percentage"></select>

            <label> $ AMOUNT</label>
            <input type="number" min="1" max="999999999" name="amount" id="amount" class="form-control" style="width: 10%">
            <label for=""># OF PAYMENTS</label>
            <input type="number" min="1" max="999" id="time" class="form-control" style="width: 10%">
            <button onclick="onClick()" target="_top" class="btn btn-light">CALCULATE!</button>
        </div>
    </div>

    <div id="result">
        <p id="show"></p>
    </div>
    <script type="text/javascript" src="./script.js"></script>
</body>

</html>

\$\endgroup\$
3
\$\begingroup\$

in the function calculateInterest() this is a bit strange :

total = total += percent;

it is either:

total += percent;

or:

total = total + percent;
\$\endgroup\$
  • \$\begingroup\$ A review should be a full review and adress all of the issues in the code. \$\endgroup\$ – FreezePhoenix Oct 1 '18 at 17:07
  • 1
    \$\begingroup\$ I'd say a rewrite without commentary engenders the same critique. But yeah, this answer is best as a comment but I expect the author does not have comment privileges yet. \$\endgroup\$ – radarbob Oct 1 '18 at 20:54
  • \$\begingroup\$ @freezePhoenix while there isn't a definitive answer on the topic, it seems that the consensus from answers to this meta post is that short answers are acceptable if they would "constitute a reasonable checkin log message"... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 2 '18 at 21:36
  • \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ Noted. Will adjust ~~schedule agenda~~ Welcoming Wagon. \$\endgroup\$ – FreezePhoenix Oct 2 '18 at 22:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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