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>

\$\endgroup\$
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>

\$\endgroup\$
  • \$\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\$ – Eric Nguyen Oct 1 '18 at 17:47
  • \$\begingroup\$ I've edited code a bit to make it more clear and have added some comments. \$\endgroup\$ – Stexxe Oct 2 '18 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 Oct 2 '18 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 Oct 2 '18 at 22:33

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.