6
\$\begingroup\$

I am new to JavaScript so I am not sure if it's bad practice to nest a for loops inside of each other.

This script simply takes a WordPress HTML menu and restructures it into a format better suited to my requirements.

Below you can see the original HTML, the JS and the new HTML output by the JS.

The nested "for of" loop checks the deepest anchor tags to see if the href matched the window and gives it a "is-active" class if it does. Should this "for of" loop be in its own function or is the nesting okay?

Is there a better way to write the JS?

Original HTML:

<aside class="menu">
  <ul class="menu-list">
    <li class="menu-label">
      <a>Label 1</a>
      <ul class="sub-menu">
        <li><a href="#">link</a></li>
        <li><a href="#">link</a></li>
        <li><a href="#">link</a></li>
        <li><a href="#">link</a></li>
        <li><a href="#">link</a></li>
      </ul>
    </li>

    <li class="menu-label">
      <a>Label 2</a>
      <ul class="sub-menu">
        <li><a href="#">link</a></li>
        <li><a href="#">link</a></li>
        <li><a href="#">link</a></li>
        <li><a href="#">link</a></li>
        <li><a href="#">link</a></li>
      </ul>
    </li>
  </ul>
</aside>

JS

const menuWrapper = document.querySelector(`aside.menu`);
const badMenu = document.querySelector(`.menu-list`);
const badMenuItems = document.querySelectorAll(`.menu-label`);

function fixMenus(menu, items) {
  // Remove old menu
  menu.remove();
  let fixedMenu = ``;

  // Iterate through and build new menu
    items.forEach(x => {
        const labels = x.children[0].innerHTML;
    const subMenus = x.children[1];
    const subItems = x.children[1].children;

    //Set "is-active" to anchor tag
    for (let i of subItems) {
      if (i.firstElementChild.href === window.location.href) {
        i.firstElementChild.classList.add(`is-active`)
      }
    }

    //Build new menu
        fixedMenu += `
      <p class="menu-label">${labels}</p>
      <ul class="menu-list">
        ${subMenus.innerHTML}
      </ul>
    `;
  });

  return fixedMenu;
}

const sideMenus = fixMenus(badMenu, badMenuItems);
menuWrapper.insertAdjacentHTML(`beforeend`, sideMenus);

NEW HTML (rebuild, returned from the JS)

<aside class="menu">
  <p class="menu-label">Label 1</p>
  <ul class="menu-list">
    <li><a class="is-active" href="#">link</a></li>
    <li><a href="#">link</a></li>
    <li><a href="#">link</a></li>
    <li><a href="#">link</a></li>
    <li><a href="#">link</a></li>
  </ul >

  <p class="menu-label">Label 2</p>
  <ul class="menu-list">
    <li><a href="#">link</a></li>
    <li><a href="#">link</a></li>
    <li><a href="#">link</a></li>
    <li><a href="#">link</a></li>
    <li><a href="#">link</a></li>
  </ul >
</aside>
\$\endgroup\$
5
  • 4
    \$\begingroup\$ Welcome to Code Review! "This script simply takes a poorly organised menu and rebuilds it." Can you tell us more about what you mean by this? What's the goal of the code? \$\endgroup\$
    – Mast
    Jun 4 '20 at 18:53
  • \$\begingroup\$ It just takes in a HTML menu and reorganises it. The original menu is structured poorly and the script changes the structure. \$\endgroup\$
    – Nick
    Jun 4 '20 at 18:56
  • \$\begingroup\$ @Mast I have edited the original post and added some more info \$\endgroup\$
    – Nick
    Jun 4 '20 at 19:00
  • 1
    \$\begingroup\$ Much better, thank you. I hope you get some good reviews. \$\endgroup\$
    – Mast
    Jun 4 '20 at 20:22
  • 2
    \$\begingroup\$ Your title should state what the code does, not your concerns about the code. Please read the relevant pages in the help center. \$\endgroup\$
    – BCdotWEB
    Jun 4 '20 at 22:24
4
\$\begingroup\$

Is there a better way to write the JS?

Yes there is.

First off, lets face it, your function fixMenus looks quite complex and a double for loop in one function is usually a big "no no" sign. Any company with a coding standard would want this to be different.

You have a double for loop with some conditions and some HTML building. That's a lot for one function and it looks hard to test accurately.

Your function does the following things

  1. Loops through each menu
  2. Destructures some data
  3. Loops through a sublist to apply a class
  4. Applies the new menu to the variable

So instead of having one function handle all this, let's break it up.

Make small simple functions that are easy to test

The concept of Test Driven Development (TDD) forces you to write code that is:

  1. Easy to understand
  2. Easy to test

This by itself already forces you to write better code and smaller functions.

You're probably not doing TDD, but writing code as if you were is already a great step!

Focus on the simple tasks

  • fixMenus should call generateMenus and ultimately generateMenu
  • Use Array#map and Array#join instead of forEach
  • create a function to update the actives
  • create a function to generate your html string
function updateActives(subItems){
   for (let i of subItems) {
      if (i.firstElementChild.href === window.location.href) {
        i.firstElementChild.classList.add(`is-active`);
      }
   }
}

function createMenu(labels, subMenus){
  return `
    <p class="menu-label">${labels}</p>
    <ul class="menu-list">
       ${subMenus}
    </ul>
  `
}

function generateMenu(item){
  const {children:[elem1, elem2]} = item;
  updateActives(elem2.children);
  return createMenu(elem1.innerHTML, elem2.innerHTML);
}

function generateMenus(items){
  return items.map(item => generateMenu(item)).join("");
}

function fixMenus(menu, items){
  menu.remove();
  return generateMenus(items);
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Awesome, thanks. I had a feeling I wasn't doing things right. I have learned a lot from this. \$\endgroup\$
    – Nick
    Jun 5 '20 at 17:14

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.