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>
2 Answers 2
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="handler()" 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="handler()" 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>
in the function calculateInterest()
this is a bit strange :
total = total += percent;
it is either:
total += percent;
or:
total = total + percent;
-
\$\begingroup\$ A review should be a full review and adress all of the issues in the code. \$\endgroup\$FreezePhoenix– FreezePhoenix2018年10月01日 17:07:47 +00:00Commented Oct 1, 2018 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– radarbob2018年10月01日 20:54:44 +00:00Commented Oct 1, 2018 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\$2018年10月02日 21:36:48 +00:00Commented Oct 2, 2018 at 21:36
-
\$\begingroup\$ @SᴀᴍOnᴇᴌᴀ Noted. Will adjust ~~schedule agenda~~ Welcoming Wagon. \$\endgroup\$FreezePhoenix– FreezePhoenix2018年10月02日 22:43:27 +00:00Commented Oct 2, 2018 at 22:43
Explore related questions
See similar questions with these tags.