Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

#DRY code

DRY code

##Integrated systems

Integrated systems

##Repeated logic

Repeated logic

##Repeated data

Repeated data

####List of value 15 as represented in your code same for 10 and 20%

List of value 15 as represented in your code same for 10 and 20%

##On the write track.

On the write track.

##In context, where it belongs, as simply as possible

In context, where it belongs, as simply as possible

###Data in the document, logic in the code.

Data in the document, logic in the code.

###Use the easy way (not the force)

Use the easy way (not the force)

###Context

Context

#DRY code

##Integrated systems

##Repeated logic

##Repeated data

####List of value 15 as represented in your code same for 10 and 20%

##On the write track.

##In context, where it belongs, as simply as possible

###Data in the document, logic in the code.

###Use the easy way (not the force)

###Context

DRY code

Integrated systems

Repeated logic

Repeated data

List of value 15 as represented in your code same for 10 and 20%

On the write track.

In context, where it belongs, as simply as possible

Data in the document, logic in the code.

Use the easy way (not the force)

Context

added 719 characters in body
Source Link
Blindman67
  • 22.8k
  • 2
  • 16
  • 40

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>

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.

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

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. MushMuch 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 (addEventListemneraddEventListener, querySelector), defined names forattributes identifying element in the DOM, HTML, and string to match these as queries and further names to hold the references in JS.

  1. fifteenPerBtn as JS named variable to hold DOM element
  2. .fifteenPercentfifteenPerBtn as JS string usedref to query DOMadd 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. fifteenPerBtn.fifteenPercent as named JS variablestring used to query DOM
  8. fifteenPercent as class name
  9. 15% as HTML content

Before this answer get to long lets take a new approach.

##In context and, where it belongs, as simply as possible

###Data onin the pagedocument, 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 add15 is displayed as "fifteen" adds a wrinkle but that is minor and such things are always good attention to details for clients.

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

Many experts say "Don't use direct referencing to access elementelements, its far better to use DOM interfaces like quereySelectorquerySelector to avoid" the "Fire and brimstone"
Using 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 nameid/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 dshortestshortest name in the context of the project that has meaning billid="bill".. If you worry about DOM / JS referencing use jQuery

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

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

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

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" >Tip 10%<>10%</button>
 <button data-tip="0.15" >Tip 15%<>15%</button>
 <button data-tip="0.2" >Tip 20%<>20%</button>
 <button data-tip="0.25" id="custBut">Tip percent<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,generouse"generous" id="tipValue"></span> percent tip is $<span id="tipAmount"></span></p>
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);
});
.hide { display : none; }
<label>Bill $</label><input id="bill" value="125.50" size=6>
<div id="tipVals">
 <button data-tip="0.1" >Tip 10%</button>
 <button data-tip="0.15" >Tip 15%</button>
 <button data-tip="0.2" >Tip 20%</button>
 <button data-tip="0.25" id="custBut">Tip percent</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>
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>

Consider the following very simple example 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

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

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. Mush 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 (addEventListemner, querySelector), defined names for DOM, HTML, and JS.

  1. fifteenPerBtn as JS named variable to hold DOM element
  2. .fifteenPercent as JS string used to query DOM
  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. fifteenPerBtn as named JS variable
  8. fifteenPercent as class name
  9. 15% as HTML content

Before this answer get to long lets take a new approach.

##In context and where it belongs

###Data on the page.

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

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

Many experts say "Don't use direct referencing to access element, its far better to use DOM interfaces like quereySelector to avoid" the "Fire and brimstone"
Using direct referencing elements via their id's or form names forces you to be aware of name uniqueness, it reduces the amount of code, and 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 dshortest name in the context of the project that has meaning bill. If you worry about DOM / JS referencing use jQuery

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

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 is meaning less, so use the shortest name that has meaning and call it tip

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

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" >Tip 10%</button>
 <button data-tip="0.15" >Tip 15%</button>
 <button data-tip="0.2" >Tip 20%</button>
 <button data-tip="0.25" id="custBut">Tip percent</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,generouse" id="tipValue"></span> percent tip is $<span id="tipAmount"></span></p>
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);
});
.hide { display : none; }
<label>Bill $</label><input id="bill" value="125.50" size=6>
<div id="tipVals">
 <button data-tip="0.1" >Tip 10%</button>
 <button data-tip="0.15" >Tip 15%</button>
 <button data-tip="0.2" >Tip 20%</button>
 <button data-tip="0.25" id="custBut">Tip percent</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>

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>

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.

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.

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.

  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

##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.

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)

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

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.

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>
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>
Source Link
Blindman67
  • 22.8k
  • 2
  • 16
  • 40

#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.

##Integrated systems

Consider the following very simple example 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

<!-- 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 are unavoidable, and the function names need to be defined and then called, thus at min require two copies, min.

##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. Mush 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 (addEventListemner, querySelector), defined names for DOM, HTML, and 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. .fifteenPercent as JS string used to query DOM
  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. fifteenPerBtn as named JS variable
  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.

Before this answer get to long lets take a new approach.

##In context and where it belongs

###Data on the page.

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

Use element.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 element, its far better to use DOM interfaces like quereySelector to avoid" the "Fire and brimstone"
Using direct referencing elements via their id's or form names forces you to be aware of name uniqueness, it reduces the amount of code, and 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 dshortest name in the context of the project that has meaning bill. If you worry about DOM / JS referencing use jQuery

(snigger 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 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 example 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" >Tip 10%</button>
 <button data-tip="0.15" >Tip 15%</button>
 <button data-tip="0.2" >Tip 20%</button>
 <button data-tip="0.25" id="custBut">Tip percent</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,generouse" 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);
});
.hide { display : none; }
<label>Bill $</label><input id="bill" value="125.50" size=6>
<div id="tipVals">
 <button data-tip="0.1" >Tip 10%</button>
 <button data-tip="0.15" >Tip 15%</button>
 <button data-tip="0.2" >Tip 20%</button>
 <button data-tip="0.25" id="custBut">Tip percent</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>

default

AltStyle によって変換されたページ (->オリジナル) /