6
\$\begingroup\$

I've built a fairly simple tip calculator. What I have so far is a bunch of functions to calculate each tip amount and calling each with eventListener.

It works however trying to see if there's a better approach that would modularize the code. I'm thinking possibilities include passing functions as arguments or functions calling other functions but can't figure it out yet.

Here's the code:

var tenPerBtn = document.querySelector('.tenPercent');
var fifteenPerBtn = document.querySelector('.fifteenPercent');
var twentyPerBtn = document.querySelector('.twentyPercent');
var customPerBtn = document.querySelector('.customTipBtn');
var bill = document.getElementById('billInput');
var tipSuggestion = document.getElementById('tipAmount');


function calcTen() {
  var billInput = bill.value;
  var tipAmount = billInput * .1;
  tipSuggestion.innerHTML = 'A ten percent tip would equal $' + tipAmount;
}

function calcFifteen() {
  var billInput = bill.value;
  var tipAmount = billInput * .15;
  tipSuggestion.innerHTML = 'A fifteen percent tip would equal $' + tipAmount;
}

function calcTwenty() {
  var billInput = bill.value;
  var tipAmount = billInput * .2;
  tipSuggestion.innerHTML = 'A twenty percent tip would equal $' + tipAmount;
}

function calcCustom() {
  var billInput = bill.value;
  var customTipAmount = document.querySelector('#customTip').value;
  var tipAmount = billInput * customTipAmount;
  tipSuggestion.innerHTML = 'A ' + customTipAmount + ' percent tip would equal $' + tipAmount;
}

tenPerBtn.addEventListener('click', calcTen)
fifteenPerBtn.addEventListener('click', calcFifteen)
twentyPerBtn.addEventListener('click', calcTwenty)
customPerBtn.addEventListener('click', calcCustom)
<label>Bill Amount: </label><input type="text" id="billInput">

<br><br>

<button class="tenPercent">Tip 10%</button>
<br>
<button class="fifteenPercent">Tip 15%</button>
<br>
<button class="twentyPercent">Tip 20%</button>
<br><br>
<label>Custom Tip Percentage: </label><input type="text" id="customTip"> <button class="customTipBtn">Tip Custom Amount</button>

<br><br>

<p id="tipAmount"></p>

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$
  • Eliminate code duplication by using function parameters (numbers in function name is usually a bad smell )
  • Separate calculating/processing data and DOM manipulations

My suggested solution:

function calcTip(bill, percent) {
    return bill * percent / 100;
}

function getMessage(tip, percent) {
    return 'A ' + percent + ' percent tip would equal $' + tip;
}

function updateFunc(billInput, tipSuggestion) {
    // Here I use closure to save billInput and tipSuggestion
    return function(percent) {
        var bill = parseFloat(billInput.value) || 0;
        tipSuggestion.innerHTML = getMessage(calcTip(bill, percent), percent);
    }
}

var tenPerBtn = document.querySelector('.tenPercent');
var fifteenPerBtn = document.querySelector('.fifteenPercent');
var twentyPerBtn = document.querySelector('.twentyPercent');
var customPerBtn = document.querySelector('.customTipBtn');
var bill = document.getElementById('billInput');
var tipSuggestion = document.getElementById('tipAmount');

var update = updateFunc(bill, tipSuggestion);

tenPerBtn.addEventListener('click', function() { update(10); });
fifteenPerBtn.addEventListener('click', function() { update(15); });
twentyPerBtn.addEventListener('click', function() { update(20); });
customPerBtn.addEventListener('click', function() { update(document.querySelector('#customTip').value); });
\$\endgroup\$
6
  • \$\begingroup\$ Took me a while to fully understand as I had to figure out what closures are. A quick YT tutorial and I've learned something new; evidently it's very useful! Feel like I powered up one level in my JS journey. Thanks for this solution! \$\endgroup\$ Commented Sep 27, 2018 at 5:35
  • \$\begingroup\$ Stexxe - can you explain your mindset/approach when programming this? I understand your code but it's a lot harder to replicate from scratch. \$\endgroup\$ Commented Sep 27, 2018 at 20:44
  • \$\begingroup\$ Ok. In each method you have code duplication. In each method you calculate some value. So, I decided to start from isolating calculation, because it's easy to start from model perspective. I created method calcTip(), which needs percent and bill amount (parameters) to calculate a tip. The same thing I did for forming a message by isolating it in the method getMessage(). That's it for model. The rest is DOM manipulations. To take input you need to have billInput, to output result - tipSuggestion, and also percent amount. So I made method update(input, output, percent) \$\endgroup\$
    – Stexxe
    Commented Sep 28, 2018 at 7:38
  • \$\begingroup\$ There are two problems with method update. 1) You need to call it few times and pass same arguments for input and output (billInput and tipSuggestion). 2) It's difficult to remember the order of parameters from function name. So I decided to save those arguments in closure to solve problem 1) and partially solve problem 2). In method update you just need to pass percent, which is only one changing. \$\endgroup\$
    – Stexxe
    Commented Sep 28, 2018 at 7:43
  • \$\begingroup\$ I think you also want to know how I would program this from scratch. You can find contacts in my profile, so we can chat and I will explain my approach to program this kind of problems. I hope this comment doesn't break any rules of service. \$\endgroup\$
    – Stexxe
    Commented Sep 28, 2018 at 8:08
2
\$\begingroup\$

DRY code

A golden coding rule is Don't Repeat Yourself (D.R.Y) which outwardly may seam a simple rule to follow but for the novice coder this is not as straight forward as it sounds. Repeated code does not always look like repeated code. You can use search and replace to change most repeated code, repeated code is seldom the same text repeated over and over.

Quick points

  • use const for constants.
  • If just setting text on an element use element.textContent not innerHTML
  • Its bad practice in JS (and most other languages) to not include a leading digit for numbers. .5 should be 0.5
  • learn the defaults for attributes so you dont need to add them. eg <input type="text"> "text" is the default type for input so the same is written <input>

Integrated systems

Consider the following very simple example (Static site) that is made up of two integrated systems, the page content (DOM/HTML/CSS) and the javascript runtime. Most frequently delivered to the client device separately. Repetition is commonly exasperated by the need to communicate between the two.

<!-- HTML -->
10 + 20 = <div id="add10_20El"></div>
30 + 40 = <div id="add30_40El"></div>
<div id="total"></div>
<script>
   function add10_20() { return 10 + 20 }   
   function add30_40() { return 40 + 30 }

   add10_20El.textContent = add10_20();
   add30_40El.textContent = add30_40();
   total = add10_20() + add30_40();
</script>

They are clearly not that repeating, a bit of a pattern (that may not always stand out), some repeated tokens (function return, etc) that is unavoidable. The function names need to be defined and then called, thus at min require two copies.

Repeated logic

One source of repetition is logic being applied. Ignoring the numbers both functions do the same thing, add values, with each function handling custom data values. Much as you have done with calcTen, calcFifteen, etc they all do the same thing, calculate a percentage and display it, but you have done it 4 times.

There is also logic that displays the results and the need for code in both HTML and JS to support this need. For your code you have the many actions (addEventListener, querySelector), attributes identifying element in the DOM, HTML, and string to match these as queries and further names to hold the references in JS.

Repeated data

There are just 4 values 10, 20, 30, 40, each repeated 7 times. In your code you have 4 values 3 fixed and one variable. If you count the number of times 15% is represented in digits, word, or named variable you also have 7 repeats, Oops make that 9

List of value 15 as represented in your code same for 10 and 20%
  1. fifteenPerBtn as JS named variable to hold DOM element
  2. fifteenPerBtn as ref to add listener
  3. calcFifteen 2 * as function name defined and called
  4. ditto above
  5. .15 as number value to do calculation with
  6. fifteen as text string to write to DOM
  7. .fifteenPercent as JS string used to query DOM
  8. fifteenPercent as class name
  9. 15% as HTML content

Not only have you repeated that value 9 times, you have done so in about as many formats as possible.

BTW In JS you can start numbers with a full stop .15 but it is considered very bad practice. For the effort of one more key the 0 is worthy readability

On the write track.

The thing you are on the right track to good dry code if you look at the custom value, it can take any percentage and format a result. At the most basic level you could have just used the click events to set the custom.value property then called the custom value handler.

In context, where it belongs, as simply as possible

Data in the document, logic in the code.

The data bill, tip percent quick options, and resulting display text and format are mostly semantic and belong on the page. The named percentages 15 is displayed as "fifteen" adds a wrinkle but that is minor and such things are always good attention to details for clients.

Use HTMLElement.dataset to add data to content when serving the page, or directly to the elements if creating content dynamically.

Use the easy way (not the force)

Many experts say "Don't use direct referencing to access elements, its far better to use DOM interfaces like querySelector to avoid" the "Fire and brimstone" or "10 reason to use jQuery" (in the history section)

Using direct referencing to access elements via their id's or form names forces you to be aware of id/name uniqueness, it reduces the amount of repeated code, thus number of lines and thus reduces bugs.

In the example I don't query for the the element called billAmount, I access it directly by its name, and I use the shortest name in the context of the project that has meaning id="bill". If you worry about DOM / JS referencing use jQuery

(sniggers its 20 years give it up..., Bill is the good guy, and IE4 had it all over NN)

Context

Code does not live in isolation, it is related to the project and has meaning implied by use and location that does not need repeating. eg customTipAmount we know its an amount so remove redundant word to get customTip, and its in a function calculating a tip, thus tip is implied, that leaves custom, which by its self is meaning less, so use the shortest name that has meaning and call it tip

Just to show that your function looses nothing with the shorter names apart from

function calcCustom() {
  var billInput = bill.value;
  var customTipAmount = document.querySelector('#customTip').value;
  var tipAmount = billInput * customTipAmount;
  tipSuggestion.innerHTML = 'A ' + customTipAmount + ' percent tip would equal $' + tipAmount;
}

becomes

// customTipAmount to tip
// tipAmount dropped
// bill as input argument
function calcCustom(bill) {
  const tip = document.querySelector('#customTip').value;
  tipSuggestion.innerHTML = 'A ' + tip + ' percent tip would equal $' + (bill * tip);
}

As this answer has feature creeped from a word or two about DRY code to TLDR partial rant the code examples below does all the above.


With some named values

var named = tipValue.dataset.named.split(",");
const tipName = (v, i = named.indexOf("" + v)) => i < 0 ? v : named[i + 1];
custom.addEventListener("change",() => custBut.dataset.tip = custom.value / 100);

tipVals.addEventListener("click", event => {
    const data = event.target.dataset;
    tipValue.textContent = tipName(data.tip * 100 + 0.5 | 0);  // lazy code + 0.5 | 0 
                                                               // rather than Math.round
    tipAmount.textContent = (data.tip * bill.value).toFixed(2);
    result.className = ""; //removes the hide
})
.hide { display : none; }
<label>Bill $</label><input id="bill" value="125.50" size=6>
<div id="tipVals">
  <button data-tip="0.1"  >10%</button>
  <button data-tip="0.15" >15%</button>
  <button data-tip="0.2"  >20%</button>
  <button data-tip="0.25" id="custBut">Custom</button>
</div>  
<label>Custom: </label>
<input id="custom" value = "25" size=4> 


<p class="hide" id="result">A <span data-named="1,one!, 5,five,10,ten,15,fifteen,20,twenty,100,generous" id="tipValue"></span> percent tip is $<span id="tipAmount"></span></p>


Without named tips

custom.addEventListener("change", () => custBut.dataset.tip = custom.value / 100);

tipVals.addEventListener("click", event => {
    const data = event.target.dataset;
    tipValue.textContent = data.tip * 100 + 0.5 | 0;
    tipAmount.textContent = (data.tip * bill.value).toFixed(2);
});
<label>Bill $</label><input id="bill" value="125.50" size=6>
<div id="tipVals">
  <button data-tip="0.1"  >10%</button>
  <button data-tip="0.15" >15%</button>
  <button data-tip="0.2"  >20%</button>
  <button data-tip="0.25" id="custBut">Custom</button>
</div>  
<label>Custom: </label>
<input id="custom" value = "25" size=4> 


<p>A <span id="tipValue">0</span> percent tip is $<span id="tipAmount">0.00</span></p>

\$\endgroup\$
3
  • \$\begingroup\$ IMHO you over complicate it a bit, but you don't have code duplication, nice. \$\endgroup\$
    – Stexxe
    Commented Sep 25, 2018 at 9:45
  • \$\begingroup\$ @Stexxe yes it is a little over the top, \$\endgroup\$
    – Blindman67
    Commented Sep 25, 2018 at 10:21
  • \$\begingroup\$ Thanks. This is a bit out of my scope of learning at this stage but great to revisit once I get more experience. \$\endgroup\$ Commented Sep 27, 2018 at 18:09

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