1
\$\begingroup\$

I wrote this Javascript snippet for a Polymer fronted, it works without issues.

The only thing that it accomplishes is changing an icon's orientation whenever a user is clicking on any expandable-item (expandable-item is a specific Tag name).

Imagine a classic Operating System's folder tree display and the + or - icon that shows if a specific folder is expandable or not.

When the user clicks, the clicked expandable-item tag receives the expanded attribute.

The user can click on several expandable-item and all of them must remain opened or closed according to the user clicks and, clearly, also the icons related to the items must change accordingly.

Are there any ways that you could suggest to make it more efficient and/or shorter?

var expandedItems = this.shadowRoot.querySelectorAll('expandable-item');

for (var i = 0; i < expandedItems.length; ++i) {
    if (expandedItems[i].hasAttribute("expanded")){
        expandedItems[i].getElementsByClassName("expandable-icon")[0].setAttribute("icon", "arrow:chevron-open-down");
    } else {
        expandedItems[i].getElementsByClassName("expandable-icon")[0].setAttribute("icon", "arrow:chevron-open-right");
    }
}

Thanks for your suggestions.

\$\endgroup\$
  • 3
    \$\begingroup\$ What task does this code accomplish? Please tell us, and also make that the title of the question via edit. Maybe you missed the placeholder on the title element: "State the task that your code accomplishes. Make your title distinctive.". Also from How to Ask: "State what your code does in your title, not your main concerns about it.". Also: Is expandable-item a tag name and/or a class name? If you are able to include sample HTML that might be helpful. Also, when does the JavaScript above get run? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Apr 25 '19 at 15:31
  • 2
    \$\begingroup\$ To make this a good question, you should include the context for this code. Are you doing anything else besides just toggling an icon? Normally, some other functionality goes along with expansion/contraction, and we could give you more proper advice if we could see the related code. \$\endgroup\$ – 200_success Apr 25 '19 at 16:15
  • \$\begingroup\$ Thanks @SᴀᴍOnᴇᴌᴀ and 200_success, I perfectly get your point! I hope the question is better now. \$\endgroup\$ – Pitto Apr 25 '19 at 20:54
  • 1
    \$\begingroup\$ Well, I don't see any change to the title so, it doesn't look better to me. Also, the text added does not answer my question regarding expandable-item being a class name, tag name or both... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Apr 25 '19 at 20:58
  • \$\begingroup\$ I applied other changes, @SᴀᴍOnᴇᴌᴀ. Thanks again for your input and patience. \$\endgroup\$ – Pitto Apr 26 '19 at 6:48
2
\$\begingroup\$

Search online for "queryselectorall vs getelementsbytagname" and you will likely see results like this SO post from August 2013 and this post from September 2010. After reading those one can posit that using Element.getElementsByTagName() instead of querySelectorAll() would be quicker, since the selector appears to merely find elements by tag name. Be aware that method returns a Live HTMLCollection so you would need to put the elements into an array before iterating over them. There are some nice tools in to do this, like using the spread syntax or calling Array.from().


If you keep the for loop, you could consider using a for...of (also standard with ) in order to eliminate the accessing array elements with bracket notation.


I am not able to find references to those attributes icon="arrow:chevron-open-down" and icon="arrow:chevron-open-right" but if those could be handled in CSS then you could eliminate the whole JS iteration block.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Precious fuel for my refactoring. Thanks for your kind help :) \$\endgroup\$ – Pitto Apr 29 '19 at 6:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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