I am practicing my javascript. I have created a link to show hide paragraphs. The code currently uses 2 'for' loops. Should I somehow be creating a function for the 'for' loop and then re-use the function?
var paragraphs = document.getElementsByTagName('p'),
firstParagraph = paragraphs[0],
link = document.createElement('a');
link.innerHTML = 'Show more';
link.setAttribute('class', 'link');
link.setAttribute('href', '#');
firstParagraph.appendChild(link);
for (var i = 1; i <= paragraphs.length - 1; i++) {
paragraphs[i].classList.add('hide')
}
function toggleHide(e) {
e.preventDefault;
var paragraphs = document.getElementsByTagName('p');
for (i = 1; i <= paragraphs.length - 1; i++) {
paragraphs[i].classList.toggle('hide');
}
}
link.addEventListener('click', toggleHide)
-
It's a matter of opinion at what point it becomes worthwhile, particularly since in this case they do slightly different things.T.J. Crowder– T.J. Crowder2017年02月27日 08:21:33 +00:00Commented Feb 27, 2017 at 8:21
-
What's not a matter of opinion (I don't think) is that you should format and indent your code readably. :-)T.J. Crowder– T.J. Crowder2017年02月27日 08:23:03 +00:00Commented Feb 27, 2017 at 8:23
2 Answers 2
Since toggle('hide') will also do the same thing of add('hide') when initializing the paragraph list, it is good to pull up the duplicate code to a single function.
For example:
var paragraphs = document.getElementsByTagName('p'),
firstParagraph = paragraphs[0],
link = document.createElement('a');
link.innerHTML = 'Show more';
link.setAttribute('class' , 'link');
link.setAttribute('href' , '#');
firstParagraph.appendChild(link);
toggleHideAll();
function toggleHide( e ){
e.preventDefault;
var paragraphs = document.getElementsByTagName('p');
toggleHideAll();
}
function toggleHideAll(){
for( i = 1 ; i <= paragraphs.length-1 ; i++){
paragraphs[i].classList.toggle('hide');
}
}
link.addEventListener( 'click' , toggleHide)
Comments
Yes, a single loop to achieve both ends would be good, as @Solmon says:
function toggleHideAll(){
for (var i = 1; i <= paragraphs.length-1; i++) {
paragraphs[i].classList.toggle('hide');
}
}
There is a more idiomatic way to express this loop, however, and I would advise you to use it, because the original form is confusing to developers who are accustomed to the standard form:
function toggleHideAll() {
for (var i = 0; i < paragraphs.length; i++) {
paragraphs[i].classList.toggle('hide');
}
}
That is, loop starting at zero, while the loop variable is less than length (not less than or equal to length minus one. And in this case, the loop does not do exactly the same as your original, because the original actually skips your first paragraph. If that's intentional, rather than tweaking the loop parameters, I would recommend toggling all of the paragraphs and then handling the special case with a line of code outside the loop:
function toggleHideAll() {
for (var i = 0; i < paragraphs.length; i++) {
paragraphs[i].classList.toggle('hide');
}
paragraphs[0].classList.remove('hide');
}
Also, it's really nice when you can avoid explicit loops in your code altogether:
function toggleHideAll() {
paragraphs.forEach(p => p.classList.toggle('hide'));
paragraphs[0].classList.remove('hide');
}