Refactoring javascript 'for' loop

Question

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. Crowder, Commented 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. Crowder, Commented Feb 27, 2017 at 8:23

Solomon Tam · Accepted Answer · 2017-02-27 08:31:10Z

1

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)

answered Feb 27, 2017 at 8:31

Solomon Tam

7393 silver badges11 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Carl Manaster · Accepted Answer · 2017-02-27 10:59:53Z

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');
}

Collectives™ on Stack Overflow

Refactoring javascript 'for' loop

2 Answers 2

Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related