Sorting alphabetically ascending and descending using .sort()

Question

I have created this basic use case of .sort() for my own reference. I am asking for a review to see if there are bad practices present or areas that could be optimised.

• Is my use of modifying the array correct? Do I need to place the newly sorted list into a new array?
• Using .map() I found it was parsing it as a string. Is insertAdjacentHTML() the right method to use here?

const listArea = document.getElementById('listArea');
const btnAsc = document.getElementById('asc');
const btnDes = document.getElementById('des');

// The Original Array
let architects = [
  "Lloyd Wright",
  "Hadid",
  "Mies van der Rohe",
  "Le Corbusier",
  "Foster",
  "Gaudi",
  "Piano",
  "Gropius",
  "Niemeyer",
  "Aalto",
  "Saarinen",
]

// Empty Array to Store Sorted Array
let sortedList = [];

// Click Events for the sort buttons
btnAsc.addEventListener('click', () => sortAsc(architects));
btnDes.addEventListener('click', () => sortDes(architects));

// Sort the array in ascending order
function sortAsc(arr) {
  // Clear the sortedList array
  sortedList.length = 0;
  
  // Convert each item in the architects array to lower case
  for(const element of arr) {
    // Push these lowercase elements into the sortedList array
    sortedList.push(element.toLowerCase());
  }
  sortedList.sort();
  populateList(sortedList);
}

function sortDes(arr) {
  sortedList.length = 0;
  for(const element of arr) {
    sortedList.push(element.toLowerCase());
  }
  sortedList.sort().reverse();
  populateList(sortedList);
}

function populateList(arr) {
  // Empty out the listArea of all content
  while(listArea.firstChild) {
    listArea.removeChild(listArea.lastChild);
  }
  // Map each item in the supplied array to an li, and remove comma at end of each
  let listArchitects = arr.map(item => `<li>${item}</li>`).join('');
  listArea.insertAdjacentHTML('beforeend', listArchitects);
}

window.addEventListener('onload', populateList(architects));

#listArea {
  text-transform: capitalize;
}

<div class="list">
  <div class="buttons">
    <button id="asc">Sort Ascending</button>
    <button id="des">Sort Descending</button>
  </div>
  <ul id="listArea">
    
  </ul>
</div>

Answer 1

Some of the things I'd point out in your code for improvements:

1. Arrays can be a const since you will not modify the array itself but just push new elements to the array.
2. In your functions sortAsc, sortDesc, you are iterating through arr just to make it lowercase and iterating through the array again in sortedList.sort(). In such scenarios, think about how you can reduce number of iterations.
3. sort() does in-place sorting, so no need to create a new array.
4. In function populateList by running arr.map you are creating another new array which I don't think is necessary.

const listArea = document.getElementById('listArea');
const btnAsc = document.getElementById('asc');
const btnDes = document.getElementById('des');

const architects = [
  "Lloyd Wright",
  "Hadid",
  "Mies van der Rohe",
  "Le Corbusier",
  "Foster",
  "Gaudi",
  "Piano",
  "Gropius",
  "Niemeyer",
  "Aalto",
  "Saarinen",
];

function sortAsc() {
  architects.sort();
  populateList();
}

function sortDes() {
  architects.sort().reverse();
  populateList();
}

function populateList() {
  listArea.innerHTML = '';
  architects.forEach( (a) => {
    const list = document.createElement('li');
    list.innerText = a;
    listArea.appendChild(list);
  });
}

btnAsc.addEventListener('click', sortAsc);
btnDes.addEventListener('click', sortDes);

window.addEventListener('onload', populateList());

<div class="list">
  <div class="buttons">
    <button id="asc">Sort Ascending</button>
    <button id="des">Sort Descending</button>
  </div>
  <ul id="listArea">
    
  </ul>
</div>

Answer 2

Before going into more detail, I noticed the following:

• The convention for naming classes is to use kebab-case. Instead of listArea I would name it list-area

• Always use const instead of let unless you need to re-assign the variable. Your array is not getting reassigned

• When adding the event to the window you are actually executing the function instead of adding an event:

  window.addEventListener('onload', populateList(architects)); // <- executes the function
  

  Also, the event is named load not onload. The reason why your code works is because the function is exectuted regardless of the event name being incorrect. So you are not actually assigning any event. It's a bug that happens to work.

  To fix this, you need to pass a function to the load event:

  window.addEventListener('load', () => populateList(architects));
  

---

Now, the approach of using insertAdjacentHTML is ok for a few items with simple markup, but as the list grows I would consider sorting the existing elements in the DOM instead of removing them and re-creating the HTML from scratch:

const ascButton = document.getElementById('asc');
const desButton = document.getElementById('des');
const listArea = document.getElementById('list-area');

const architects = [
  "Lloyd Wright",
  "Hadid",
  "Mies van der Rohe",
  "Le Corbusier",
  "Foster",
  "Gaudi",
  "Piano",
  "Gropius",
  "Niemeyer",
  "Aalto",
  "Saarinen",
];

// Create elements using the DOM instead of HTML strings
const architectElems = architects.map(architect => {
  const item = document.createElement('li');
  item.textContent = architect;
  return item;
});

function sort(order = 'asc') {
  const sorted = architectElems.sort((a, b) => {
    if (order === 'asc') {
      return a.textContent.localeCompare(b.textContent);
    }
    return b.textContent.localeCompare(a.textContent);
  });
  // We can append the same elements to the container
  // to reorder them without having to remove them from the DOM
  // and recreate the HTML
  listArea.append(...sorted);
}

ascButton.addEventListener('click', () => sort('asc'));
desButton.addEventListener('click', () => sort('des'));

window.addEventListener('load', () => {
  // Add elements to list for the first time
  listArea.append(...architectElems);
});
```