2
\$\begingroup\$

I've written a simple script that takes an array, sorts it (by omitting the articles) and displays it on a list. But I feel like this code is really huge, bulky and messy and could be simplified!

What I'm asking for:

  • What would be a more efficient way to do what I'm trying to do?
  • How would I make this code more neat (or compact in other words).
  • Could any new JS features ease up my work?

<!DOCTYPE html>
<html lang="en">

<head>
    <meta charset="UTF-8">
    <title>Sort Without Articles</title>
</head>

<body>

    <style>
        body {
            margin: 0;
            font-family: sans-serif;
            background: url("https://source.unsplash.com/nDqA4d5NL0k/2000x2000");
            background-size: cover;
            display: flex;
            align-items: center;
            min-height: 100vh;
        }
        
        #bands {
            list-style: inside square;
            font-size: 20px;
            background: white;
            width: 500px;
            margin: auto;
            padding: 0;
            box-shadow: 0 0 0 20px rgba(0, 0, 0, 0.05);
        }
        
        #bands li {
            border-bottom: 1px solid #efefef;
            padding: 20px;
        }
        
        #bands li:last-child {
            border-bottom: 0;
        }
        
        a {
            color: #ffc600;
            text-decoration: none;
        }
    </style>

    <ul id="bands"></ul>

    <script>
        const bandsList = document.querySelector('#bands');

        const bands = ['The Plot in You', 'The Devil Wears Prada', 'Pierce the Veil', 'Norma Jean', 'The Bled', 'Say Anything', 'The Midway State', 'We Came as Romans', 'Counterparts', 'Oh, Sleeper', 'A Skylit Drive', 'Anywhere But Here', 'An Old Dog'];

        const articles = ['The', 'An', 'A'];

        const sorted = bands.sort((previous, next) => {
            previous = previous.split(' ');
            next = next.split(' ');

            if (articles.includes(previous[0])) {
                previous = previous.splice(1, previous.length).join(' ');
            } else {
                previous = previous.join(' ');
            }


            if (articles.includes(next[0])) {
                next = next.splice(1, previous.length).join(' ');
            } else {
                next = next.join(' ');
            }

            if (previous < next) return -1;
            if (previous > next) return 1;

            return 0;
        });

        const markup = sorted.reduce((markup, name) => {
            return `
              ${markup}
              <li>${name}</li>
            `;
        }, '');

        bandsList.innerHTML = markup;
    </script>

</body>

</html>

\$\endgroup\$
2
\$\begingroup\$

I would consider using a regex instead:

const articlesRegex = /^(The|An|A)\s+/i;

const sorted = bands.sort((previous, next) => {
  previous = previous.replace(articlesRegex,'');
  next = next.replace(articlesRegex,'');

You should also return 0 when the items are equal.

\$\endgroup\$
1
\$\begingroup\$

You can exploit map and method chaining to make it much shorter.

<!DOCTYPE html>
<html lang="en">

<head>
  <meta charset="UTF-8">
  <title>Sort Without Articles</title>
</head>

<body>

  <style>
    body {
      margin: 0;
      font-family: sans-serif;
      background: url("https://source.unsplash.com/nDqA4d5NL0k/2000x2000");
      background-size: cover;
      display: flex;
      align-items: center;
      min-height: 100vh;
    }
    
    #bands {
      list-style: inside square;
      font-size: 20px;
      background: white;
      width: 500px;
      margin: auto;
      padding: 0;
      box-shadow: 0 0 0 20px rgba(0, 0, 0, 0.05);
    }
    
    #bands li {
      border-bottom: 1px solid #efefef;
      padding: 20px;
    }
    
    #bands li:last-child {
      border-bottom: 0;
    }
    
    a {
      color: #ffc600;
      text-decoration: none;
    }
  </style>

  <ul id="bands"></ul>

  <script>
    const bandsList = document.querySelector('#bands');

    const bands = ['The Plot in You', 'The Devil Wears Prada', 'Pierce the Veil', 'Norma Jean', 'The Bled', 'Say Anything', 'The Midway State', 'We Came as Romans', 'Counterparts', 'Oh, Sleeper', 'A Skylit Drive', 'Anywhere But Here', 'An Old Dog'];

    bandsList.innerHTML = bands
      .map(text => ({
        sort: text.replace(/^(The|An|A)\s+/i, ''),
        text
      }))
      .sort(({sort:a}, {sort:b}) => a.localeCompare(b))
      .reduce((markup, {text}) =>
            `
              ${markup}
              <li>${text}</li>
            `, '');
  </script>

</body>

</html>

\$\endgroup\$
  • \$\begingroup\$ The outputs don't match ... The original code preserves the articles in the sorted output but ignores them for sorting purposes, which is different from what you did. \$\endgroup\$ – Vogel612 Aug 5 '17 at 10:49
  • \$\begingroup\$ Welcome to Code Review! It's worth noting that this uses the decorate-sort-undecorate technique, also known as the Schwartzian transform. It's more efficient than the original code because it avoids repeating the string manipulation with each comparison. \$\endgroup\$ – 200_success Aug 5 '17 at 23:48

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.