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%
fifteenPerBtn
as JS named variable to hold DOM element
fifteenPerBtn
as ref to add listener
calcFifteen
2 * as function name defined and called
- ditto above
.15
as number value to do calculation with
fifteen
as text string to write to DOM
.fifteenPercent
as JS string used to query DOM
fifteenPercent
as class name
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>