I'm creating a page where users can purchase an adult, child or senior ticket, each with it's own pricing.
Here's the code for review. Please let me know if there are ways to improve upon this code.
// declare all variables
var adultQty = document.getElementById('quantityAdult');
var childQty = document.getElementById('quantityChild');
var seniorQty = document.getElementById('quantitySenior');
var submitBtn = document.getElementById('submitButton');
var outputPara = document.getElementById('totalPrice');
// generic function that takes in quantity and multiplies with appropriate price
function calcPrice(qty, price){
return qty * price;
}
// generic function that outputs final price and amout it tickets purchased
function getMessage(qty, total){
return outputPara.innerHTML = 'You purchased ' + qty + ' ticket(s) and your total price is $' + total + '<br><br>' + '<button>Proceed To Checkout</button>';
}
submitBtn.addEventListener('click', function() {
if(adultQty.value === '0' && childQty.value === '0' && seniorQty.value === '0'){
alert('Please purchase at least 1 ticket');
} else {
var totalAdult = calcPrice(adultQty.value, 49);
var totalChild = calcPrice(childQty.value, 20);
var totalSenior = calcPrice(seniorQty.value, 30);
var totalPrice = totalAdult + totalChild + totalSenior;
var totalTix = parseInt(adultQty.value) + parseInt(childQty.value) + parseInt(seniorQty.value);
getMessage(totalTix, totalPrice);
}
});
<p>Purchase your tickets online! </p>
<ul>
<li>49ドル - Adult</li>
<li>20ドル - Child</li>
<li>30ドル - Senior </li>
</ul>
<label>Quantity: </label><input type="text" id="quantityAdult" value="0"> <label>Adult</label>
<br><br>
<label>Quantity: </label><input type="text" id="quantityChild" value="0"> <label>Child</label>
<br><br>
<label>Quantity: </label><input type="text" id="quantitySenior" value="0"> <label>Senior</label>
<br><br>
<button type="submit" id="submitButton">Submit</button>
<p id="totalPrice"></p>
1 Answer 1
General advices
Separate pure functions from impure ones. Pure functions don't have side effects and they are easy to test and reuse. If you have function, which contain pure (some calculations) and impure (DOM access/modify) parts — split it on two functions.
Create functions, which will do only thing. Do not force reader (primarily you) to keep in mind function side effects.
To improve code quality start by writing functions calls instead of functions definitions. This will force you to think more about function signature and its usability.
Solve more general problem after solving few particular ones. General solutions are more reusable, although could be more complex.
Line by line review
- Comment
// declare all variables
is not very useful. This comment contains obvious information. - Comment
// generic function that takes in quantity and multiplies with appropriate price
discloses inner working of functioncalcPrice()
. Reader should not understand how function work in order to use it. - Your function name is
calcPrice()
and its second parameter isprice
. This is confusing. You actually calculate total price. - Comment
// generic function that outputs final price and amout it tickets purchased
is redundant and contains typo. - Function
getMessage()
has two responsibilities. First one is creating string message, second one is assigning it toinnerHTML
of HTML element. Split it. - You will get
NaN
if one of input fields is empty. - Don't define var variables in block scope.
var
has function scope. Use IIFE if you need to have a scope. - You duplicate function call
calcPrice()
three times, which is not really bad, but can be improved.
Suggested solution
function calcTotalPrice(quantity, price) {
return quantity * price;
}
function getMessage(quantity, totalPrice) {
return 'You purchased ' + quantity + ' ticket(s) and your total price is $' + totalPrice
}
function parseQuantity(val) {
return parseInt(val, 10) || 0;
}
// Sum numbers in given list
function sum(list) {
return list.reduce(function(acc, x) {
return acc + x;
}, 0)
}
(function() {
// We use IIFE here to define a scope to initialize some variables here
var submitBtn = document.getElementById('submitButton');
var outputPara = document.getElementById('totalPrice');
var config = [
[document.getElementById('quantityAdult'), 49],
[document.getElementById('quantityChild'), 20],
[document.getElementById('quantitySenior'), 30]
];
submitBtn.addEventListener('click', function() {
var totalPrices;
var quantities = config.map(function (data) { // In ES6 we could use array destructing: [el, price]
var el = data[0];
return parseQuantity(el.value);
});
if ( sum(quantities) > 0 ) {
totalPrices = config.map(function(data) {
var el = data[0], price = data[1];
return calcTotalPrice(parseQuantity(el.value), price);
});
outputPara.innerHTML = getMessage( sum(quantities), sum(totalPrices) );
} else {
alert('Please purchase at least 1 ticket');
}
});
}());
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<title>Title</title>
</head>
<body>
<p>Purchase your tickets online! </p>
<ul>
<li>49ドル - Adult</li>
<li>20ドル - Child</li>
<li>30ドル - Senior </li>
</ul>
<label>Quantity: </label><input type="text" id="quantityAdult" value="0"> <label>Adult</label>
<br><br>
<label>Quantity: </label><input type="text" id="quantityChild" value="0"> <label>Child</label>
<br><br>
<label>Quantity: </label><input type="text" id="quantitySenior" value="0"> <label>Senior</label>
<br><br>
<button type="submit" id="submitButton">Submit</button>
<p id="totalPrice"></p>
<br><br>
<button>Proceed To Checkout</button>
<script src="script.js"></script>
</body>
</html>
-
\$\begingroup\$ To clarify:
parseQuantity()
converts to integer,sum()
adds up ticket quantity by usingreduce()
method,mapElements()
creates new array with updated price ? And can you elaborate on why you used IIFE function? Thanks - lots of concepts that are new to me, but having fun breaking your code down and learning from it. \$\endgroup\$Eric Nguyen– Eric Nguyen2018年10月01日 17:47:59 +00:00Commented Oct 1, 2018 at 17:47 -
\$\begingroup\$ I've edited code a bit to make it more clear and have added some comments. \$\endgroup\$Stexxe– Stexxe2018年10月02日 06:26:12 +00:00Commented Oct 2, 2018 at 6:26
-
\$\begingroup\$ If I removed the IIFE and assigned a variable name to that function, then called that function, it would still work. Besides redundant task of assigning name and calling that function, what is the benefit of IIFE? Is it because we only need to call the function once? \$\endgroup\$Eric Nguyen– Eric Nguyen2018年10月02日 18:00:15 +00:00Commented Oct 2, 2018 at 18:00
-
\$\begingroup\$ Overall, very good info. Especially creating functions that only perform ONE function. The biggest issue with this approach, like you mentioned, is it does create for a more complex main/general function. But this approach seems like a best practice and separates newbies like me from seasoned developer like you. I will use this approach with my next project. Thanks again! \$\endgroup\$Eric Nguyen– Eric Nguyen2018年10月02日 22:33:58 +00:00Commented Oct 2, 2018 at 22:33