2
\$\begingroup\$

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>

200_success
145k22 gold badges190 silver badges478 bronze badges
asked Sep 30, 2018 at 14:26
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

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 function calcPrice(). Reader should not understand how function work in order to use it.
  • Your function name is calcPrice() and its second parameter is price. 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 to innerHTML 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>

answered Sep 30, 2018 at 18:21
\$\endgroup\$
4
  • \$\begingroup\$ To clarify: parseQuantity() converts to integer, sum() adds up ticket quantity by using reduce() 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\$ Commented Oct 1, 2018 at 17:47
  • \$\begingroup\$ I've edited code a bit to make it more clear and have added some comments. \$\endgroup\$ Commented 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\$ Commented 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\$ Commented Oct 2, 2018 at 22:33

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.